Add --olm-namespace flag to OLM related subcommands
#2613
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
size/M
|
hey guys, I am going to do some little rework after thinking through questions @camilamacedo86 asked. I think I made changes more than needed. pls hold off review until then. I'l post one done. |
internal/olm/operator/operator.go
Outdated
| @@ -97,6 +100,8 @@ func (c *OLMCmd) AddToFlagSet(fs *pflag.FlagSet) { | |||
| "These supplement or override defaults generated by run/cleanup") | |||
| fs.DurationVar(&c.Timeout, "timeout", defaultTimeout, | |||
| prefix+"Time to wait for the command to complete before failing") | |||
| fs.StringVar(&c.OLMNamespace, "olm-namespace", olm.DefaultNamespace, | |||
| prefix+"OLM Namespace") | |||
| } | |||
|
|
|||
| func (c *OLMCmd) validate() error { | |||
There was a problem hiding this comment.
Validate whether OLMCmd.OLMNamespace is set.
There was a problem hiding this comment.
OLMNamespace is currently optional, and it defaults to olm.DefaultNamespace(olm) , not sure what validation should be done?, can you pls provide an example if i'm missing anything
There was a problem hiding this comment.
It defaults to olm.DefaultNamespace in its flags, but what if OLMCmd is populated directly in code? OLMCmd.OLMNamespace isn't defaulted anywhere AFAIK, and is used directly by operatorManager methods.
Instead of validating, just default to olm.DefaultNamespace in newManager().
There was a problem hiding this comment.
ah yes. added it now.
|
@camilamacedo86 @estroz addressed the review comments, pls review it. |
jeyaramashok
changed the title
--olm-namespace flag to OLM related subcommands--olm-namespace flag to OLM related subcommands
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
pls let me know if I need to rebase the commits |
There was a problem hiding this comment.
Hi @jeyaramashok,
Really thank you for the contribution. Added a few comments regards nits required. Could you please rebase and check them? Otherwise, it shows lgtm for me.
openshift-ci-robot
added
the
needs-rebase
openshift-ci-robot
removed
the
needs-rebase
|
rebased the commits |
openshift-ci-robot
removed
the
needs-rebase
|
@camilamacedo86 I have address your comments in 6cfe531, let me know if the changes look okay or need any further changes. I'l rebase after that ( since its easy to look at changes with commits) |
jeyaramashok
force-pushed
the
fix_2595
branch
2 times, most recently
from
1c0bc41
to
79fffe2
Compare
openshift-ci-robot
added
the
needs-rebase
openshift-ci-robot
removed
lgtm
|
rebased the PR with master, and made a minor change in the flag section, to move |
|
@jeyaramashok we just added website docs to the SDK repo, and some docs need to be duplicated. Run $ cp ./doc/cli/* ./website/content/en/docs/cli/to duplicate the cli docs changes. |
|
copied the docs to other directory. I found |
|
Looks like the go string literals in command files have bad indentation. Feel free to revert the indentation changes, or change indentation in the go filed themselves. I will make a follow up PR in the former case. |
|
New changes are detected. LGTM label has been removed. |
|
@estroz fixed the indentation in source file |
cmd/operator-sdk/new/cmd.go
Outdated
| @@ -53,48 +53,48 @@ generates a default directory layout based on the input <project-name>. | |||
| <project-name> is the project name of the new operator. (e.g app-operator) | |||
| `, | |||
| Example: `# Create a new project directory | |||
There was a problem hiding this comment.
Last nit (should have mentioned this before): would be nice to have two spaces indenting each examples line:
| Example: `# Create a new project directory | |
| Example: ` # Create a new project directory |
and so on.
Then the printout of examples aligns with Usage:
$ operator-sdk new --help
The operator-sdk new command creates a new operator application and
generates a default directory layout based on the input <project-name>.
<project-name> is the project name of the new operator. (e.g app-operator)
Usage:
operator-sdk new <project-name> [flags]
Examples:
# Create a new project directory
$ mkdir $HOME/projects/example.com/
$ cd $HOME/projects/example.com/
...There was a problem hiding this comment.
ok done. now I understand why it was indented before :)
- Add '--olm-namespace' flag to to 'operator-sdk run --olm' command - Add '--olm-namespace' flag to 'operator-sdk olm status' command - Add '--olm-namespace' flag to 'operator-sdk cleanup' command This fixes the bug where running these commands against a openshift 4.x cluster would fail because in those clusters the OLM is installed in 'namesapce: openshift-operator-lifecycle-manager' and the code always defaulted to default 'namespace: olm'. Its now fixed by adding a new flag '--olm-namespace' that allows to override the default namespace (olm). Fixes: operator-framework#2595
Description of the change:
--olm-namespaceflag to tooperator-sdk run --olmcommand--olm-namespaceflag tooperator-sdk olm statuscommand--olm-namespaceflag tooperator-sdk cleanupcommandMotivation for the change:
This fixes the bug where running these commands against a openshift 4.x cluster would fail because in those clusters the OLM is installed in
namesapce: openshift-operator-lifecycle-managerand the code always defaulted to defaultnamespace: olm. Its now fixed by adding a new flag--olm-namespacethat allows to override the default namespace(olm).Closes: #2595
Example usage