Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add schemaVersion to porter.yaml #1946

Merged
merged 7 commits into from
Mar 11, 2022

Conversation

carolynvs
Copy link
Member

@carolynvs carolynvs commented Feb 27, 2022

What does this change

This adds a document schema version to the Porter manifest (porter.yaml) similar to what we already have on credential and parameter sets. The schema of the any document should not change within a major version.

I've added a page that lists which schema versions are used by various porter versions.

Preview of doc changes at https://deploy-preview-1946--porter.netlify.app/reference/file-formats/

What issue does it fix

Closes #1566

Notes for the reviewer

I've created #1956 as a follow-up so that we start using the schemaVersion set.

Checklist

  • Did you write tests?
  • Did you write documentation?
  • Did you change porter.yaml or a storage document record? Update the corresponding schema file.
  • If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇‍♀️

Reviewer Checklist

  • Comment with /azp run test-porter-release if a magefile or build script was modified
  • Comment with /azp run porter-integration if it's a non-trivial PR

@carolynvs
Copy link
Member Author

/azp run porter-integration

@azure-pipelines
Copy link

Pull request contains merge conflicts.

@carolynvs carolynvs force-pushed the manifest-schema-version branch 2 times, most recently from def2ab2 to 0c74d27 Compare February 28, 2022 16:45
This adds a document schema version to the Porter manifest (porter.yaml)
similar to what we already have on credential and parameter sets.
The schema of the any document should not change within a major
version.

I've added a page that lists which schema versions are used by various
porter versions.

Signed-off-by: Carolyn Van Slyck <[email protected]>
All of our other file format schemas are in pkg/schema. This moves the
file to the right location and adds a copy instruction in our build
target to copy the template from the schema directory when we build.

Make the schema available at /schema/v1/NAME.schema.json

Signed-off-by: Carolyn Van Slyck <[email protected]>
@carolynvs carolynvs force-pushed the manifest-schema-version branch from 0c74d27 to 95c97f4 Compare March 2, 2022 18:44
Signed-off-by: Carolyn Van Slyck <[email protected]>
@carolynvs carolynvs changed the title Manifest schema version Add schemaVersion to porter.yaml Mar 2, 2022
@carolynvs
Copy link
Member Author

/azp run porter-integration

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

I'm avoiding returning a hard error immediately because then people would upgrade and no existing
bundles would work for them. Maybe in the future we can change this to a hard check when more
people have had a chance to migrate.

Signed-off-by: Carolyn Van Slyck <[email protected]>
@carolynvs
Copy link
Member Author

/azp run porter-integration

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@carolynvs carolynvs marked this pull request as ready for review March 2, 2022 22:35
@carolynvs carolynvs requested a review from VinozzZ March 2, 2022 22:36
| schemaType | false | The type of document. This isn't used by Porter but is included when Porter outputs the file, so that editors can determine the resource type. |
| schemaVersion | true | The version of the Bundle schema used in this file. |
| name | true | The name of the bundle. |
| version | false | The version of the bundle, must adher to [semver v2]. |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| version | false | The version of the bundle, must adher to [semver v2]. |
| version | false | The version of the bundle, must adhere to [semver v2]. |

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we mention what value will be used for the bundle's tag if version isn't provided? Just tested and it appears to be a SHA of some sort (which isn't in itself semver, right?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed the docs to indicate that version is required, though people can set it programmatically during build thanks to the feature you added a while back. So it's not possible to avoid setting version.

The bundle still uses a tag defaulted using the version unless you set it with the reference field, or --tag when publishing.

What you are probably noticing is Yingrong's recent fix for how we first push the invocation image (which used to be named BUNDLENAME-installer), retrieved its repository digest and then patched the bundle.json before publishing the bundle. We now push the invocation image to a tag (a hash) in the same repository as the bundle. We still do the same trick to update the repository digest in the bundle before publishing. The difference is that we don't create additional repositories that are unused (i.e. the -installer image isn't actually used by the bundle, we use the one in BUNDLENAME@sha256:abc123). This makes it more clear to people that we are generating stuff that they shouldn't try to reference by name and that the image is distributed with the bundle.

Untagging so that we don't leave stuff around to confuse people isn't supported yet but is on the backlog.

docs/content/reference/file-formats.md Outdated Show resolved Hide resolved
magefile.go Outdated Show resolved Hide resolved
pkg/templates/templates.go Outdated Show resolved Hide resolved
@VinozzZ
Copy link
Contributor

VinozzZ commented Mar 3, 2022

Looks great to me!
One thing that I'm not sure is when a dev makes a schema change, do we have a way to let them know that they also need to increase the supported schema version?

@carolynvs
Copy link
Member Author

One thing that I'm not sure is when a dev makes a schema change, do we have a way to let them know that they also need to increase the supported schema version?

That's a great idea. I'll add someone about breaking changes to CONTRIBUTING.MD, as I think it's a general topic. Then also add some comments inline so people know "If you change this data structure, here's what you have to do next..."

Signed-off-by: Carolyn Van Slyck <[email protected]>
@carolynvs
Copy link
Member Author

@vdice Updated with everyone's review feedback. Please take another look when you can!

Copy link
Member

@vdice vdice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

CONTRIBUTING.md Outdated
## Breaking Changes

Some changes in Porter break our compatibility with previous versions of Porter.
When that happens, we need to release that change a new major version number to indicate to users that it contains breaking changes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
When that happens, we need to release that change a new major version number to indicate to users that it contains breaking changes.
When that happens, we need to release that change with a new major version number to indicate to users that it contains breaking changes.

docs/content/reference/file-formats.md Outdated Show resolved Hide resolved
Signed-off-by: Carolyn Van Slyck <[email protected]>
@carolynvs carolynvs merged commit 38b8d18 into getporter:release/v1 Mar 11, 2022
@carolynvs carolynvs deleted the manifest-schema-version branch March 11, 2022 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include compatible porter version in porter.yaml
3 participants