-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
generate (new CLI): change default output dirs #3257
generate (new CLI): change default output dirs #3257
Conversation
2bce804
to
a1e7d15
Compare
/hold Blocked by #3258 |
kustomize
subcommand, change default output dirs
/remove-kind feature |
a1e7d15
to
87dce21
Compare
…elated files; instead, `generate kustomize manifests` will do so. `bundle` will write manifests/metadata to the top-level `bundle/` directory, and `packagemanifests` will write manifests and the package manifest to the top-level `packagemanifests/`; these are deployable files that do not require further kustomization, so should not reside in `config/`. *: remove `config/` prefix for generated manifests/metadata, change `config/{bundle,packagemanifests}` to `config/manifests` in `--kustomize` logic
87dce21
to
eadf808
Compare
/hold cancel |
if c.kustomizeDir == "" { | ||
return errors.New("--kustomize-dir must be set") |
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.
So --kustomize-dir
seems to be a required flag now. Unless it should be defaulted to config/manifests
.
Can we mark it as such, and also I didn't see it used in the example text:
$ kustomize build config/manifests | operator-sdk generate bundle --manifests --metadata --overwrite --version 0.0.1
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.
This flag is required but defaults to config/manifests
.
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.
Just double checking. It is required in the sense that it can't be empty, but not required in the sense that it doesn't have to be provided on the command line?
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.
Correct, much like --deploy-dir
in the legacy CLI.
22f054d
to
cba74f0
Compare
cba74f0
to
fd0fa16
Compare
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:
*: remove
config/
prefix for generated manifests/metadata for both bundles and package manifestsMotivation for the change:
bundle
andpackagemanifests
default output paths are changing because generated files do not require further kustomization and so should not reside inconfig/
with other kustomizeable manifests.bundle
will write manifests/metadata to the top-levelbundle/
directorypackagemanifests
will write manifests and the package manifest to the top-levelpackagemanifests/
directoryThis PR also leverages
generate kustomize manifests
, added in #3258./cc @hasbro17 @joelanford @jmrodri @bharathi-tenneti
/kind feature