Skip to content

Conversation

varshaprasad96
Copy link
Member

@varshaprasad96 varshaprasad96 commented Aug 7, 2020

Description of the change:
Remove 'olm-namespace' flag from 'operator-sdk olm install' command.

Motivation for the change:
The olm manifests published in github have a hardcoded
namespace value. Hence, the olm operators can only be installed in olm
namespace using operator-sdk olm install command.

Checklist

If the pull request includes user-facing changes, extra documentation is required:

Remove 'olm-namespace' flag from 'operator-sdk olm install' command.

Reason: The OLM manifests which are used to install olm-operator have
hardcoded namespace value.
@estroz estroz added this to the v1.0.0 milestone Aug 7, 2020
@@ -35,8 +35,6 @@ func newInstallCmd() *cobra.Command {
}

cmd.Flags().StringVar(&mgr.Version, "version", olm.DefaultVersion, "version of OLM resources to install")
cmd.Flags().StringVar(&mgr.OLMNamespace, "olm-namespace", olm.DefaultOLMNamespace,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mgr.OLMNamespace is still used in InstallVersion. We should remove this field from the manager's struct and pass the hard-coded default olm.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mgr.OLMNamespace is still used by status and uninstall commands though

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m.initialize() would fill the DefaultOLMNamespace value in manager's struct. So at least for install command, the mgr.OLMNamespace would always be olm when InstallVersion is called.

Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 7, 2020
@jmrodri
Copy link
Member

jmrodri commented Aug 7, 2020

Help looks good

[jesusr@transam operator-sdk{olm-test}]$ build/operator-sdk olm install --help
Install Operator Lifecycle Manager in your cluster

Usage:
  operator-sdk olm install [flags]

Flags:
  -h, --help               help for install
      --timeout duration   time to wait for the command to complete before failing (default 2m0s)
      --version string     version of OLM resources to install (default "latest")

Global Flags:
      --verbose   Enable verbose logging

The flag is gone so trying to use it will print the usage.

[jesusr@transam operator-sdk{olm-test}]$ build/operator-sdk olm install --olm-namespace default
Error: unknown flag: --olm-namespace
Usage:
  operator-sdk olm install [flags]

Flags:
  -h, --help               help for install
      --timeout duration   time to wait for the command to complete before failing (default 2m0s)
      --version string     version of OLM resources to install (default "latest")

Global Flags:
      --verbose   Enable verbose logging

FATA[0000] unknown flag: --olm-namespace                

Copy link
Member

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@joelanford joelanford merged commit a8f5e00 into operator-framework:master Aug 7, 2020
@varshaprasad96 varshaprasad96 deleted the olm/remove-namespace branch August 7, 2020 02:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants