Skip to content
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

<run|cleanup> packagemanifests: remove --olm-namespace, change --operator-namespace to --namespace #3601

Merged

Conversation

estroz
Copy link
Member

@estroz estroz commented Jul 29, 2020

Description of the change:

  • internal/olm/operator: remove OLM namespace usage, OLM installation detection, create resources in provided operator namespace, and change operator namespace to namespace
  • website: update docs

Motivation for the change: <run|cleanup packagemanifests flag --olm-namespace has been removed since it is no longer used by the command, and --operator-namespace has been renamed to --namespace. OLM-adjacent resources (ex. registry deployment) are now created in the provided namespace, and OLM installation is no longer checked.

/cc @joelanford @jmrodri @rashmigottipati

Checklist

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

@estroz estroz added the olm-integration Issue relates to the OLM integration label Jul 29, 2020
@joelanford joelanford mentioned this pull request Jul 29, 2020
92 tasks
Copy link
Member

@rashmigottipati rashmigottipati left a comment

Choose a reason for hiding this comment

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

/lgtm

after fixing typo in log message and CI passes

internal/olm/operator/packagemanifests_manager.go Outdated Show resolved Hide resolved
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2020
@estroz estroz force-pushed the chore/remove-run-olm-namespace branch from 60dd523 to 6265bd4 Compare July 29, 2020 20:04
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2020
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

@@ -0,0 +1,15 @@
entries:
- description: Removed the `--olm-namespace` flag from `run packagemanifests`.
Copy link
Member

Choose a reason for hiding this comment

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

+1 to using the past tense, Removed, it reads better when the changelog is generated.

header: Remove `--olm-namespace` from `run packagemanifests` invocations
body: >
OLM namespace is no longer required by this command.
- description: Changed the `--operator-namespace` flag to `--namespace` in `run packagemanifests`.
Copy link
Member

Choose a reason for hiding this comment

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

+1 to Changed

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

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

/lgtm

@jmrodri
Copy link
Member

jmrodri commented Jul 29, 2020

@estroz test broke

Unexpected error:

        <*errors.errorString | 0xc00003e480>: {

            s: "operator-sdk run packagemanifests --install-mode AllNamespaces --operator-namespace e2e-dvvb-system --version 0.0.1 --timeout 4m failed with error: Error: unknown flag: --operator-namespace\nUsage:\n  operator-sdk run packagemanifests [flags]\n\nAliases:\n  packagemanifests, pm\n\nFlags:\n  -h, --help                  help for packagemanifests\n      --install-mode string   InstallMode to create OperatorGroup with. Format: InstallModeType[=ns1,ns2[, ...]]\n      --kubeconfig string     The file path to kubernetes configuration file. Defaults to location specified by $KUBECONFIG, or to default file rules if not set\n      --namespace string      The namespace where operator resources are created. It must already exist in the cluster\n      --timeout duration      Time to wait for the command to complete before failing (default 2m0s)\n      --version string        Packaged version of the operator to deploy\n\nGlobal Flags:\n      --verbose   Enable verbose logging\n\ntime=\"2020-07-29T20:37:23Z\" level=fatal msg=\"unknown flag: --operator-namespace\"\n",```

@rashmigottipati
Copy link
Member

One clarification: we probably need to change this from --operator-namespace to --namespace in e2e tests too, right?

…ed since it

is no longer used by the command, and `--operator-namespace` has been renamed to
`--namespace`.
@estroz estroz force-pushed the chore/remove-run-olm-namespace branch from 6265bd4 to af455e1 Compare July 29, 2020 20:56
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2020
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@estroz estroz merged commit 1ab7291 into operator-framework:master Jul 29, 2020
@estroz estroz deleted the chore/remove-run-olm-namespace branch July 29, 2020 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
olm-integration Issue relates to the OLM integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants