Introduce initial version 2 and stronger versioning support for the spec#384
Introduce initial version 2 and stronger versioning support for the spec#384jsoriano merged 22 commits intoelastic:mainfrom
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
🌐 Coverage report
|
mtojek
left a comment
There was a problem hiding this comment.
I went back and forth through this PR a few times as it's a relatively big one, but it brings the schema patching feature. I haven't noticed anything concerning or blocking.
LGTM!
| func TestBundledSpecsForIntegration(t *testing.T) { | ||
| fs := spec.FS() | ||
| _, err := fs.Open("1/integration/spec.yml") | ||
| _, err := fs.Open("integration/spec.yml") |
There was a problem hiding this comment.
Most likely it will break the docs repository. You will need another PR to fix it. FYI it isn't a major problem.
There was a problem hiding this comment.
Thanks, I will prepare the follow-up.
There was a problem hiding this comment.
I have updated the pull request template in this repository, and opened PRs in the integrations and docs repositories. Also added a placeholder to avoid broken links: https://github.com/elastic/package-spec/blob/26925a80d08a39cd0d188ba81bc71b09464b1d04/versions/1/README.md
| func CheckVersion(version *semver.Version) (*semver.Version, error) { | ||
| d, err := fs.ReadFile(content, "spec/changelog.yml") | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
Is it safe to panic here? Is it executed only during init?
There was a problem hiding this comment.
I followed the same approach as in FS() when accessing the filesystem. But yeah, it'd be better to don't panic here.
|
/test |
mrodm
left a comment
There was a problem hiding this comment.
This approach to get a new specification version (V2) looks promising! 🚀
I've been thinking in some scenarios like removing fields (as in the license property example) or change allowed values for some property (it would be a replace operation), and it would fit perfectly 👍
| specification for a package's contents. It describes the the folder structure of packages and expected | ||
| files within these folders (this is point 1. above). The specification is expressed using a schema simil | ||
| ar to [JSON Schema](https://json-schema.org/), but with a couple of differences: | ||
| -- The `type` field can be either `folder` or `file`, |
There was a problem hiding this comment.
| specification for a package's contents. It describes the the folder structure of packages and expected | |
| files within these folders (this is point 1. above). The specification is expressed using a schema simil | |
| ar to [JSON Schema](https://json-schema.org/), but with a couple of differences: | |
| -- The `type` field can be either `folder` or `file`, | |
| specification for a package's contents. It describes the folder structure of packages and expected | |
| files within these folders (this is point 1. above). The specification is expressed using a schema similar | |
| to [JSON Schema](https://json-schema.org/), but with a couple of differences: | |
| -- The `type` field can be either `folder` or `file`, |
| for pkgName, expectedErrorMessage := range tests { | ||
| t.Run(pkgName, func(t *testing.T) { | ||
| errs := ValidateFromPath(filepath.Join("..", "..", "..", "..", "test", "packages", pkgName)) | ||
| errs := ValidateFromPath(path.Join("..", "..", "..", "..", "test", "packages", pkgName)) |
|
I have done more updates on the README to asjust the text to current structure. And added tests to ensure that possible conflicts in patches are detected for all versions defined in changelog. |
What does this PR do?
This PR introduces some changes in how versioning is done in the spec:
format_versionis strongly checked now, it must exist in the specchangelog.yml, prereleases of the version are allowed.changelog.next.ymlfile for features only enabled for next major.versionslist, with patches to apply to reconstruct older versions of the schema.licensefield is removed from manifests to illustrate that.Why is it important?
To allow introducing breaking changes (or loosening validation) in a more controlled way.
Checklist
test/packagesthat prove my change is effective.versions/N/changelog.yml.Related issues