Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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

@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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
olm-integration Issue relates to the OLM integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants