Introduce initial version 2 and stronger versioning support for the spec #384

jsoriano · 2022-07-28T11:35:05Z

What does this PR do?

This PR introduces some changes in how versioning is done in the spec:

Versioned spec directories are removed.
format_version is strongly checked now, it must exist in the spec changelog.yml, prereleases of the version are allowed.
Multiple prereleases may exist in the changelog file, one for each major. This allows to keep development of multiple majors in a single branch. Some alternatives for this:
- Keep a separated changelog.next.yml file for features only enabled for next major.
- Have a branch per major, and use backports.
Semantic rules can be selected by version now. Validation of unique fields definition (Enable check to avoid duplicated fields #331) has been re-enabled to illustrate that.
YAML Schema files can now include a versions list, with patches to apply to reconstruct older versions of the schema. license field 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

I have added test packages to test/packages that prove my change is effective.
I have added an entry in versions/N/changelog.yml.

Related issues

Alternative to Introduce format version v2 #380.
Closes Enable check to avoid duplicated fields #331.
Closes Introduce new package spec version #345.

elasticmachine · 2022-07-28T11:42:18Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-08-23T14:04:41.850+0000
Duration: 8 min 22 sec

Test stats 🧪

Test	Results
Failed	0
Passed	585
Skipped	0
Total	585

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.

elasticmachine · 2022-07-28T11:42:27Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	100.0% (`7/7`)	💚
Files	70.833% (`17/24`)	👍
Classes	78.788% (`26/33`)	👍
Methods	58.824% (`60/102`)	👍 0.408
Lines	44.629% (`536/1201`)	👍 0.754
Conditionals	100.0% (`0/0`)	💚

mtojek

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!

mtojek · 2022-08-22T12:59:11Z

code/go/internal/spec_test.go

@@ -14,6 +14,12 @@ import (

 func TestBundledSpecsForIntegration(t *testing.T) {
 	fs := spec.FS()
-	_, err := fs.Open("1/integration/spec.yml")
+	_, err := fs.Open("integration/spec.yml")


Most likely it will break the docs repository. You will need another PR to fix it. FYI it isn't a major problem.

Thanks, I will prepare the follow-up.

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

mtojek · 2022-08-22T13:03:07Z

spec.go

+func CheckVersion(version *semver.Version) (*semver.Version, error) {
+	d, err := fs.ReadFile(content, "spec/changelog.yml")
+	if err != nil {
+		panic(err)


Is it safe to panic here? Is it executed only during init?

I followed the same approach as in FS() when accessing the filesystem. But yeah, it'd be better to don't panic here.

mtojek · 2022-08-22T13:16:18Z

/test

mrodm

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 👍

mrodm · 2022-08-22T14:34:26Z

README.md

+ 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`,


Suggested change

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`,

mrodm · 2022-08-22T14:35:50Z

code/go/pkg/validator/validator_test.go

 	tests := map[string]string{
 		"bad_duplicated_fields": "field \"event.dataset\" is defined multiple times for data stream \"wrong\", found in: ../../../../test/packages/bad_duplicated_fields/data_stream/wrong/fields/base-fields.yml, ../../../../test/packages/bad_duplicated_fields/data_stream/wrong/fields/ecs.yml",
 	}

 	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))


jsoriano · 2022-08-23T09:57:23Z

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.

mrodm

👍

README.md

mrodm

👍

jsoriano added 4 commits July 28, 2022 12:41

Get rid of the versioned spec

38e85ff

Versioned validation rules, re-enable check for duplicated fields

9bd43d4

Remove specPath

0760853

Check format version with changelog

bad0afc

jsoriano added the discuss Issue needs discussion label Jul 28, 2022

jsoriano self-assigned this Jul 28, 2022

jsoriano mentioned this pull request Jul 28, 2022

Introduce format version v2 #380

Closed

2 tasks

Try to fix test in Windows

51ee005

jsoriano mentioned this pull request Aug 8, 2022

Introduce new package spec version #345

Closed

Merge remote-tracking branch 'origin/main' into format-version-unique

8460778

jsoriano mentioned this pull request Aug 15, 2022

Use CUE to define schema for files #391

Closed

4 tasks

jsoriano added 6 commits August 16, 2022 14:35

Add support for patches in spec files

7dd4b76

Versioned json schema

accfb2b

Add test package

4d05da9

Remove license also in input

e891c65

Add comments

aabc366

Add changelog entry for the removal of license

b337fa6

jsoriano marked this pull request as ready for review August 17, 2022 10:30

jsoriano requested a review from a team as a code owner August 17, 2022 10:30

Merge remote-tracking branch 'origin/main' into format-version-unique

19d02e5

mtojek previously approved these changes Aug 22, 2022

View reviewed changes

jsoriano added 2 commits August 22, 2022 16:11

Fixes after merge with master

f37c7dc

Don't panic when checking spec version

2f876b3

jsoriano dismissed mtojek’s stale review via 2f876b3 August 22, 2022 14:14

mrodm previously approved these changes Aug 22, 2022

View reviewed changes

jsoriano added 2 commits August 23, 2022 10:33

Updates in README

6f26ca8

Add more tests

9685592

jsoriano dismissed mrodm’s stale review via 9685592 August 23, 2022 09:53

Another fix in README

74c8e1c

jsoriano requested review from mrodm and mtojek August 23, 2022 09:57

Update PR template

7d4e662

This was referenced Aug 23, 2022

Update links in README for spec elastic/integrations#4053

Merged

Update link to future package spec elastic/observability-docs#2119

Merged

jsoriano added 2 commits August 23, 2022 12:18

Fix test

a4ca49b

Keep a placeholder in the versions directory

26925a8

mrodm previously approved these changes Aug 23, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

Remove another reference to versions

8962247

jsoriano dismissed mrodm’s stale review via 8962247 August 23, 2022 14:04

jsoriano requested a review from mrodm August 23, 2022 14:04

mrodm approved these changes Aug 23, 2022

View reviewed changes

jsoriano merged commit a6b8e9d into elastic:main Aug 24, 2022

jsoriano deleted the format-version-unique branch August 24, 2022 09:22

This was referenced Aug 24, 2022

Prepare version 1.16.0 #397

Merged

Re-enable validation of duplicated fields #338

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce initial version 2 and stronger versioning support for the spec #384

Introduce initial version 2 and stronger versioning support for the spec #384

jsoriano commented Jul 28, 2022 •

edited

Loading

elasticmachine commented Jul 28, 2022 •

edited

Loading

Build stats

Test stats 🧪

elasticmachine commented Jul 28, 2022 •

edited

Loading

mtojek left a comment

mtojek Aug 22, 2022

jsoriano Aug 22, 2022 •

edited

Loading

jsoriano Aug 23, 2022

mtojek Aug 22, 2022

jsoriano Aug 22, 2022

mtojek commented Aug 22, 2022

mrodm left a comment

mrodm Aug 22, 2022

mrodm Aug 22, 2022

jsoriano commented Aug 23, 2022

mrodm left a comment

mrodm left a comment

Introduce initial version 2 and stronger versioning support for the spec #384

Introduce initial version 2 and stronger versioning support for the spec #384

Conversation

jsoriano commented Jul 28, 2022 • edited Loading

What does this PR do?

Why is it important?

Checklist

Related issues

elasticmachine commented Jul 28, 2022 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

elasticmachine commented Jul 28, 2022 • edited Loading

🌐 Coverage report

mtojek left a comment

Choose a reason for hiding this comment

mtojek Aug 22, 2022

Choose a reason for hiding this comment

jsoriano Aug 22, 2022 • edited Loading

Choose a reason for hiding this comment

jsoriano Aug 23, 2022

Choose a reason for hiding this comment

mtojek Aug 22, 2022

Choose a reason for hiding this comment

jsoriano Aug 22, 2022

Choose a reason for hiding this comment

mtojek commented Aug 22, 2022

mrodm left a comment

Choose a reason for hiding this comment

mrodm Aug 22, 2022

Choose a reason for hiding this comment

mrodm Aug 22, 2022

Choose a reason for hiding this comment

jsoriano commented Aug 23, 2022

mrodm left a comment

Choose a reason for hiding this comment

mrodm left a comment

Choose a reason for hiding this comment

jsoriano commented Jul 28, 2022 •

edited

Loading

elasticmachine commented Jul 28, 2022 •

edited

Loading

elasticmachine commented Jul 28, 2022 •

edited

Loading

jsoriano Aug 22, 2022 •

edited

Loading