-
Notifications
You must be signed in to change notification settings - Fork 191
feat(deploy): support for package namespace override flag #3889
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Brandt Keller <[email protected]>
Signed-off-by: Brandt Keller <[email protected]>
Signed-off-by: Brandt Keller <[email protected]>
✅ Deploy Preview for zarf-docs canceled.
|
Signed-off-by: Brandt Keller <[email protected]>
Signed-off-by: Brandt Keller <[email protected]>
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.
A few nits on an overall simple & elegant solution
Signed-off-by: Brandt Keller <[email protected]>
Signed-off-by: Brandt Keller <[email protected]>
Signed-off-by: Brandt Keller <[email protected]>
Signed-off-by: Brandt Keller <[email protected]>
Signed-off-by: Brandt Keller <[email protected]>
Signed-off-by: Brandt Keller <[email protected]>
Signed-off-by: Brandt Keller <[email protected]>
Signed-off-by: Brandt Keller <[email protected]>
Signed-off-by: Brandt Keller <[email protected]>
Signed-off-by: Brandt Keller <[email protected]>
Signed-off-by: Brandt Keller <[email protected]>
Signed-off-by: Brandt Keller <[email protected]>
…de support Signed-off-by: Brandt Keller <[email protected]>
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.
I like the change in direction, a few suggestions + calls for unit tests
src/config/lang/english.go
Outdated
|
||
CmdPackageMirrorFlagComponents = "Comma-separated list of components to mirror. This list will be respected regardless of a component's 'required' or 'default' status. Globbing component names with '*' and deselecting components with a leading '-' are also supported." | ||
CmdPackageMirrorFlagNoChecksum = "Turns off the addition of a checksum to image tags (as would be used by the Zarf Agent) while mirroring images." | ||
|
||
CmdPackageInspectFlagSbomOut = "Specify an output directory for the SBOMs from the inspected Zarf package" | ||
CmdPackageInspectFlagListImages = "List images in the package (prints to stdout)" | ||
CmdPackageInspectFlagNamespace = "[Alpha] Override the namespace for package inspection. Requires the package to have been deployed using the namespace override." |
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.
Requires feels like the wrong verb here. Maybe something like Applicable only to in-cluster packages deployed to a dynamic namespace
. Ditto for remove
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.
That is fair! modified to potentially meet this intent.
src/cmd/package.go
Outdated
if opts.NamespaceOverride != "" { | ||
nsCount := pkgLayout.Pkg.GetUniqueNamespaceCount() | ||
if nsCount > 1 { | ||
return nil, fmt.Errorf("package contains %d namespaces, cannot override namespace to %s", nsCount, opts.NamespaceOverride) | ||
} | ||
pkgLayout.Pkg.SetPackageNamespace(opts.NamespaceOverride) | ||
} |
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.
I get why we validate and set the package namespace early here, but we should also validate and set the package namespace at the CLI level. Currently, Deploy accepts the NamespaceOverride
opt, but if someone is calling the function purely at the SDK level, then it will only have an impact on the secret, not on the namespace the package is deployed to. To avoid duplication, one option would be to make a common function in packager2 both deploy and the cli can call.
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.
Great points - I tested a few different strategies for locality of the namespace override identifiers and ultimately landed on your thoughts as well. Isolating the logic in a common function and then duplicating across CLI/SDK to meet parity while primarily isolating the core logic to DeployOptions
and Deploy()
.
Signed-off-by: Brandt Keller <[email protected]>
Signed-off-by: Brandt Keller <[email protected]>
Signed-off-by: Brandt Keller <[email protected]>
Signed-off-by: Brandt Keller <[email protected]>
Signed-off-by: Brandt Keller <[email protected]>
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.
One small nit, otherwise lgtm
@@ -24,6 +24,7 @@ zarf package remove { PACKAGE_SOURCE | PACKAGE_NAME } --confirm [flags] | |||
--components string Comma-separated list of components to remove. This list will be respected regardless of a component's 'required' or 'default' status. Globbing component names with '*' and deselecting components with a leading '-' are also supported. | |||
--confirm REQUIRED. Confirm the removal action to prevent accidental deletions | |||
-h, --help help for remove | |||
-n, --namespace string [Alpha] Override the namespace for package removal. Applicable only to packages deployed using the namespace override flag. |
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.
If we're adding the -n
shorthand we should add it to the other commands. Also we should just say the "namespace flag" instead of "namespace override flag" imo.
Signed-off-by: Brandt Keller <[email protected]>
@Racer159 you raised a good point that a constraint behind this is that it does not support init-packages to-date. We'd need to rewire all of the hardcoded assumptions around the |
Signed-off-by: Brandt Keller <[email protected]>
Description
This pull request adds support for a
--namespace
flag in the deploy operation - allowing users with a single-namespace package to deploy the package multiple times to multiple isolated namespaces. This is the implementation for proposal ZEP-0017.Constraints
ZarfInitConfig
packages.Design Choices
ZarfPackage
methods for validating a package has only declared a single namespace & updating all package components to the target namespaceNamespaceOverride
as a new field underDeployedPackage
NamespaceOverride
field with json tags allows us to store it in the package state secret. Any call tocluster.GetDeployedZarfPackages
will have the information innately.package name
solely for secret identification.zarf-package-my-package-override-namespacename
such that the collisions should only occur in a very specific edgecase.DeployedPackage
and remaining "k8s" types tostate
package.Related Issue
Fixes #2407
Checklist before merging