-
Notifications
You must be signed in to change notification settings - Fork 173
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
feat: deprecate and replace zarf package inspect
with child commands zarf package inspect definition|sbom|images
#3416
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
✅ Deploy Preview for zarf-docs canceled.
|
Codecov ReportAttention: Patch coverage is
|
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
zarf package inspect
to a parent command, zarf package inspect definition|sbom|images
zarf package inspect
to a parent command, zarf package inspect definition|sbom|images
zarf package inspect
to a parent command, zarf package inspect definition|sbom|images
zarf package inspect
with child commands zarf package inspect definition|sbom|images
Signed-off-by: Austin Abro <[email protected]>
src/cmd/package.go
Outdated
definitionOpts := PackageInspectDefinitionOptions{ | ||
SkipSignatureValidation: pkgConfig.PkgOpts.SkipSignatureValidation, | ||
} | ||
return definitionOpts.Run(cmd, args) |
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.
Good stuff 👍
src/cmd/package.go
Outdated
RunE: o.Run, | ||
} | ||
|
||
cmd.Flags().BoolVar(&o.SkipSignatureValidation, "skip-signature-validation", false, lang.CmdPackageFlagSkipSignatureValidation) |
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 haven't implemented that bit yet, but ideally you want to have NewPackageInspectSBOMOptions
function which folks can invoke and get default options for a command. Then when you wire your flags here instead of providing defaults directly, you're just using the field from options struct.
A nice example of that is in kubectl api-resources
command. Notice how NewAPIResourceOptions
is passed with IO streams, and then each flag is automatically wired to the default value of a particular options.
The main benefit of such an approach is that you have only a single source of truth wrt defaults.
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 went ahead and implemented this
src/cmd/package.go
Outdated
} | ||
|
||
// NewPackageInspectSBOMCommand creates the `package inspect sbom` sub-command. | ||
func NewPackageInspectSBOMCommand(v *viper.Viper) *cobra.Command { |
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.
Same comment as in #3413 about exporting these functions.
return err | ||
} | ||
|
||
cluster, _ := cluster.NewCluster() //nolint:errcheck |
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.
Whenever explicitly ignoring error it's good to explain why you're doing so. I'm guessing that since this is only showing the contents of the file we don't care about the error.
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.
We actually don't know at this point yet if a cluster is needed, which is why we avoid the error. Added context in comments
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Description
feat: change
zarf package inspect
to a parent command,zarf package inspect definition|sbom|images
This also removes the ability to view SBOMs directly in browser with the
--sbom
flagRelated Issue
ZEP#9
Checklist before merging