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

check for plugins file schema version and type #2532

Merged
merged 4 commits into from
Jan 25, 2023

Conversation

VinozzZ
Copy link
Contributor

@VinozzZ VinozzZ commented Jan 24, 2023

What does this change

Previously, the SchemaType and SchemaVersion are not parsed or validated. This PR adds the corresponding logic to properly process these fields

What issue does it fix

Notes for the reviewer

Put any questions or notes for the reviewer here.

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

@VinozzZ VinozzZ force-pushed the implement-schema-version-and-type branch 3 times, most recently from 62cd202 to 5ab29af Compare January 24, 2023 14:32
Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
@VinozzZ VinozzZ force-pushed the implement-schema-version-and-type branch from 5ab29af to 40fb44e Compare January 24, 2023 15:58
Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

Since this is changing the file format used from what was just published in v1.0.5, we'll need to communicate that this is a breaking change and update the release notes on 1.0.5 to call out that it's a bad release effectively.

In addition to the usual announcements, since this is a breaking change (and a bad release), you should draft a blog post to go out at the same time as the new release that explains what went wrong, which version to use, and provides an explanation of the install multiple plugins feature. I recommend saying that's its a great way to quickly set up your Porter environment with your required plugins so that people get an idea of when you would use this feature. Probably don't mention the Porter Operator change that caused us to want this feature since it's not shipped yet, so let's focus on how people can use it right now.

docs/content/reference/file-formats.md Show resolved Hide resolved
if o.Version != "" {
return fmt.Errorf("plugin version should not be specified when --file is provided")
// version should not be set to anything other than the default value
if o.Version != "" && o.Version != "latest" {
Copy link
Member

Choose a reason for hiding this comment

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

When --version is empty, it defaults to latest so I'm not sure I get what this is checking for or guarding against? The error message just says that they can't specify --version at all so can't we just stick with the original check that version was specified?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When no version is specified by users, the o.Version is default to latest. The original check would fail bc the o.Version is not empty.
The check here is to guard that users didn't try to set a version value through the flag. o.Version should not have value other than "latest" or empty

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! I didn't realize that we were defaulting to "latest" so early, I'm assuming it's the flag default. Makes sense! 👍

}

func (spec InstallPluginsSpec) Validate() error {
if InstallPluginsSchemaVersion != schema.Version(spec.SchemaVersion) {
Copy link
Member

Choose a reason for hiding this comment

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

I just remembered that we are missing schemaType validation on our other resources. I had created a branch to fix that and then got pulled off onto other things and forgot to submit a PR.

Let's do a check here for the schemaType as well, so that if they enter in a value other than what's expected, we don't try to process the file further. For example, they accidentally pass a bundle or installation file to the -f parameter.

https://github.com/carolynvs/porter/blob/ea694aa668a448eb2293775da5f5cfda3567214f/pkg/manifest/manifest.go#L185-L187

I'll submit my PR afterwards that adds the check to the other resources.

Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
"plugins": {
"description": "The definition of plugins to install",
"type": "object",
"default": "1.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

This default shouldn't be defined here. I assume you meant to define it directly on schemaVersion?

},
"feedURL": {
"description": "The URL of an atom feed where the plugin can be downloaded.",
"key": {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is how the properties json schema field works for objects. Have you tested this against the json schema validator (https://www.jsonschemavalidator.net/) with both matching and incorrect schema to make sure it's correct?

Here is how I have defined an object with unknown keys in porter elsewhere (for custom action metadata):

https://github.com/getporter/porter/blob/2aa80a6670bd0aac7284cb8f5ad375b6b44b3365/pkg/schema/manifest.schema.json#L417C6-L421

"type": "object",
"properties": {
"version": {
"description": "The version for the plugin.",
Copy link
Member

Choose a reason for hiding this comment

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

I like to give the user hints that will show up in the hover text / documentation that will help them understand how to fill out the file better:

Suggested change
"description": "The version for the plugin.",
"description": "The version for the plugin. Defaults to latest when unspecified",

"type": "string"
},
"feedURL": {
"description": "The URL of an atom feed where the plugin can be downloaded.",
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
"description": "The URL of an atom feed where the plugin can be downloaded.",
"description": "The URL of an atom feed where the plugin can be downloaded. Defaults to the official Porter plugin feed.",

"type": "string"
},
"url": {
"description": "The URL from where the plugin can be downloaded",
Copy link
Member

Choose a reason for hiding this comment

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

This field is hard to guess, so I always like to include an example of how to download from github releases:

Suggested change
"description": "The URL from where the plugin can be downloaded",
"description": "The URL from where the plugin can be downloaded. For example, https://github.com/MChorfa/porter-helm3/releases/download",

"value": {
},
"plugins": {
"description": "The definition of plugins to install",
Copy link
Member

Choose a reason for hiding this comment

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

It would help to give people a hint that the field names under plugins should be the name of the plugin

Suggested change
"description": "The definition of plugins to install",
"description": "A map of plugins to install, keyed by the plugin name",

Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
Copy link
Member

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

Looks great!

This reminds me that we need to update the porter vs code extension to check for schemaType and apply the proper json schema to the current document, e.g. if the open document is for porter and has "schemaType: Plugins", then vs code should apply this plugins json schema so that the user gets autocomplete.

I'll create another issue to work out the details but wanted to let you know how this schema will be used elsewhere too. 👍

@carolynvs
Copy link
Member

Here's the issue to track showing autocomplete using our other json schema documents: getporter/vscode-extension#47

Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
@VinozzZ VinozzZ merged commit 9343f85 into getporter:main Jan 25, 2023
@VinozzZ VinozzZ deleted the implement-schema-version-and-type branch January 25, 2023 16:34
carolynvs pushed a commit that referenced this pull request Jan 30, 2023
* Refactor PrintLintResults() to ensure lint warnings are printed.

Signed-off-by: James Blair <mail@jamesblair.net>

* Bump github.com/moby/buildkit from 0.11.0 to 0.11.1 (#2528)

Bumps [github.com/moby/buildkit](https://github.com/moby/buildkit) from 0.11.0 to 0.11.1.

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* check for plugins file schema version and type (#2532)

* check for plugins file schema version and type

Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>

* Update contributors list.

Signed-off-by: James Blair <mail@jamesblair.net>

* Add unit tests for ensuring lint warnings are printed.

Signed-off-by: James Blair <mail@jamesblair.net>

---------

Signed-off-by: James Blair <mail@jamesblair.net>
Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Yingrong Zhao <yingrong.zhao@gmail.com>
bdegeeter pushed a commit to bdegeeter/porter that referenced this pull request May 11, 2023
* check for plugins file schema version and type

Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
bdegeeter pushed a commit to bdegeeter/porter that referenced this pull request May 11, 2023
* Refactor PrintLintResults() to ensure lint warnings are printed.

Signed-off-by: James Blair <mail@jamesblair.net>

* Bump github.com/moby/buildkit from 0.11.0 to 0.11.1 (getporter#2528)

Bumps [github.com/moby/buildkit](https://github.com/moby/buildkit) from 0.11.0 to 0.11.1.

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* check for plugins file schema version and type (getporter#2532)

* check for plugins file schema version and type

Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>

* Update contributors list.

Signed-off-by: James Blair <mail@jamesblair.net>

* Add unit tests for ensuring lint warnings are printed.

Signed-off-by: James Blair <mail@jamesblair.net>

---------

Signed-off-by: James Blair <mail@jamesblair.net>
Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Yingrong Zhao <yingrong.zhao@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Yingrong Zhao <yingrong.zhao@gmail.com>
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.

2 participants