-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
manifest generation: remove metadata.namespace
from manifests
#3813
manifest generation: remove metadata.namespace
from manifests
#3813
Conversation
@joelanford do you know which validator is returning this error? Also I’m not sure why this is a requirement at all. Much like for a CSV, OLM should be able to substitute whatever namespace it wants into a bundled manifest regardless of whether one is set or not. Is this so operator authors won’t get the idea that their object will be installed into the namespace specified in the manifest? @dinhxuanvu @gallettilance thoughts on this? Relates to #3748. |
/hold For discussion about whether this validator is appropriate (and if it is, I want to add some unit tests to prevent regressions) |
From @dinhxuanvu:
|
@@ -212,6 +212,7 @@ func (c *Manifests) addRoles(rawManifests ...[]byte) error { | |||
if err := yaml.Unmarshal(rawManifest, &role); err != nil { | |||
return err | |||
} | |||
role.SetNamespace("") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be easier to just remove an object's namespace before writing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, the package and naming of WriteObjectsToFiles
makes me hesitate a little putting this logic there since it seems like a very general-purpose and obvious function. I interpret it as "please serialize these objects directly to files".
WDYT of removing the namespace of all objects just before they're passed into this function and only when we're using it in the context of writing a manifests directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, how about here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 to your last comment.
…ng namespaces in add* functions
…ernal/manifests.go
7e932da
to
e548936
Compare
/hold cancel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Description of the change:
When generating bundles and packagemanifests, remove
metadata.namespace
from namespaced resources when writing them into themanifests
directory to avoid validation errors.OLM installs namespaced manifests in the namespace that the operator is installed, so it is invalid to hardcode a specific namespace in these manifests.
Motivation for the change:
Closes #3809
Checklist
If the pull request includes user-facing changes, extra documentation is required:
changelog/fragments
(seechangelog/fragments/00-template.yaml
)Add or update relevant sections of the docs website inwebsite/content/en/docs