-
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: remove --operator-name
#3530
generate: remove --operator-name
#3530
Conversation
095cef2
to
f4bc418
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 👍
f4bc418
to
4875e9b
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.
One (repeated) question.
Otherwise
/lgtm
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.
IMO it is blocked by #3341.
Note that in #3341 we are centralizing the code to getOperatorName where has the validations suggested by @joelanford in the review made here. And then, we can:
- Merge Allows OLM subcommands work for Helm/Ansible new layout #3341.
- Rebase it with master
- This pr will only change do the changes to replace the
--operator-name
fromgenerate
subcommands in favor of usingproject-name
- Also, then the changes performed here will be tested and we will be able to ensure that nothing breaks the OLM features for Go a NoGo operators as well. (before the Allows OLM subcommands work for Helm/Ansible new layout #3341 we cannot test it via CI)
PS.: Note that the goal of #3341 is to fixes OLM subcommands for NoGo operators and add the tests to ensure that all will work well as expected.
projectName
over --operator-name
, and remove flag--operator-name
4875e9b
to
2faf035
Compare
35077ce
to
bf0d53d
Compare
… be used instead
bf0d53d
to
1ae7c2c
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
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.
Minor nit if you have to fix other things. If not ignore. And a question to make sure we meant to remove the lower casing.
# release notes and/or the migration guide | ||
entries: | ||
- description: > | ||
Remove `--operator-name` from `generate` subcommands in favor of using `project-name` |
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.
Nit, not a PR blocker: I ended up having to update these to be past tense Removed
because the changelog read weird with just Remove
. For the migration guide Remove
makes sense since it's an instruction.
@@ -173,7 +172,7 @@ func (g Generator) makeBaseGetter(inputDir string) func() (*apimanifests.Package | |||
|
|||
// makePkgManFileName will return the file name of a PackageManifest. | |||
func makePkgManFileName(operatorName string) string { | |||
return strings.ToLower(operatorName) + packageManifestFileExt | |||
return operatorName + packageManifestFileExt |
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.
Do we no longer need to lowercase this?
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 should assume operatorName
is lowercased at this point.
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
--operator-name
flagMotivation for the change:
projectName
is now universal for plugins, so should be used instead of a flag value./cc @joelanford @camilamacedo86 @hasbro17
Checklist
If the pull request includes user-facing changes, extra documentation is required:
changelog/fragments
(seechangelog/fragments/00-template.yaml
)website/content/en/docs