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

Namespace Package Verify #1536

Merged
merged 2 commits into from
May 26, 2020
Merged

Namespace Package Verify #1536

merged 2 commits into from
May 26, 2020

Conversation

kensipe
Copy link
Member

@kensipe kensipe commented May 22, 2020

Package Verify now includes namespace checks

  1. No kind: Namespace allowed in an operator
  2. namespace manifest is specified must exist, must be 1 manifest in the yaml file, it must be kind: Namespace

Signed-off-by: Ken Sipe kensipe@gmail.com

Fixes #

Signed-off-by: Ken Sipe <kensipe@gmail.com>
@kensipe kensipe added the release/highlight This PR is a highlight for the next release label May 22, 2020
Copy link
Contributor

@zen-dog zen-dog left a comment

Choose a reason for hiding this comment

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

Lgtm. However, we should mark one of these KEP-31 related PRs as release/highlight to get the changelog visibility with a lengthy description of the feature and with release/breaking because this is sort-of breaking: using namespaces resources used to work and now it doesn't.

labels:
app: my-app`

pf := packages.Files{
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be a table test and you would "save" ~50 slocs 😉

}
ns, err := renderer.YamlToObject(val)
if err != nil {
res.AddErrors(fmt.Sprintf("Unable to marshal NamespaceManifest %q ", pf.Operator.NamespaceManifest))
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
res.AddErrors(fmt.Sprintf("Unable to marshal NamespaceManifest %q ", pf.Operator.NamespaceManifest))
res.AddErrors(fmt.Sprintf("Unable to marshal NamespaceManifest %q ", pf.Operator.NamespaceManifest))
return

We probably want to abort checking here, 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.

agree! thank you!

Comment on lines +50 to +52
// it would be great if we could marshal the templates into an unstructured runtime.Object to confirm Namespace
// however with templating we don't have the ability to have all required information to do so.
// best effort is to confirm that "kind: Namespace" is not used.
Copy link
Member

Choose a reason for hiding this comment

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

It might be an idea in the future to let the operator developer to provide an example-params.yaml in the operator development that provides values for all required parameters so we can render the templates and verify more thoroughly...

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 like that idea... however :). in cases where there are conditional branches in the templates... you would only be able to evaluate the one in the example :( which still has the likelihood of missing constraints like this.

@kensipe kensipe added the release/breaking-change This PR contains breaking changes and is marked in the release notes label May 26, 2020
Signed-off-by: Ken Sipe <kensipe@gmail.com>
@kensipe kensipe merged commit cdbbade into master May 26, 2020
@kensipe kensipe deleted the ken/ns-verify branch May 26, 2020 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release/breaking-change This PR contains breaking changes and is marked in the release notes release/highlight This PR is a highlight for the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants