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

cmd/operator-sdk: OLM integration alpha run/cleanup CLI #2402

Merged
merged 5 commits into from
Jan 15, 2020

Conversation

estroz
Copy link
Member

@estroz estroz commented Jan 13, 2020

Description of the change: 'olm up/down' commands will deploy and delete an operator using OLM via manifests stored in an in-cluster registry-server managed by the SDK.

  • cmd/operator-sdk/alpha/olm: add operator up/down commands

Motivation for the change: operator-sdk should provide tools to interact with OLM. See #1912 for past discussion of CLI.

Closes #1912.

using OLM via manifests stored in a registry-server.

cmd/operator-sdk/alpha/olm: add operator up/down commands
@estroz estroz added the olm-integration Issue relates to the OLM integration label Jan 13, 2020
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 13, 2020
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 13, 2020
@estroz estroz changed the title cmd/operator-sdk: olm up/down CLI cmd/operator-sdk: OLM integration alpha run/cleanup CLI Jan 14, 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 after a couple of nits and minor questions.

CHANGELOG.md Outdated
@@ -2,6 +2,8 @@

### Added

- Added [`run`](./doc/cli/operator-sdk_alpha_run.md) amd [`cleanup`](./doc/cli/operator-sdk_alpha_cleanup.md) subcommands (under the `alpha` subcommand) to manage deployment/deletion of operators. These commands currently interact with OLM via an in-cluster registry-server created using an operator's on-disk manifests and managed by `operator-sdk`. ([#2402](ttps://github.com/operator-framework/operator-sdk/pull/2402))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Added [`run`](./doc/cli/operator-sdk_alpha_run.md) amd [`cleanup`](./doc/cli/operator-sdk_alpha_cleanup.md) subcommands (under the `alpha` subcommand) to manage deployment/deletion of operators. These commands currently interact with OLM via an in-cluster registry-server created using an operator's on-disk manifests and managed by `operator-sdk`. ([#2402](ttps://github.com/operator-framework/operator-sdk/pull/2402))
- Added [`run`](./doc/cli/operator-sdk_alpha_run.md) and [`cleanup`](./doc/cli/operator-sdk_alpha_cleanup.md) subcommands (under the `alpha` subcommand) to manage deployment/deletion of operators. These commands currently interact with OLM via an in-cluster registry-server created using an operator's on-disk manifests and managed by `operator-sdk`. ([#2402](ttps://github.com/operator-framework/operator-sdk/pull/2402))

### Options

```
--force-registry Force deletion of the in-cluster registry. This option is a no-op on 'up'.
Copy link
Member

Choose a reason for hiding this comment

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

Curious about this: This option is a no-op on 'up'.

Does up need to be run? But that would make it seem like it's always a no-op. I'm a little confused...

Copy link
Member Author

Choose a reason for hiding this comment

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

This flag should be specific to cleanup so I've moved it to cleanup.NewCmd().

@@ -36,7 +36,7 @@ const (
type OLMCmd struct { // nolint:golint
// ManifestsDir is a directory containing a package manifest and N bundles
// of the operator's CSV and CRD's. OperatorVersion can be set to the
// version of the desired operator version's subdir and Up()/Down() will
// version of the desired operator version's subdir and Run()/Down() will
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// version of the desired operator version's subdir and Run()/Down() will
// version of the desired operator version's subdir and Run()/Cleanup() will

```
--force-registry Force deletion of the in-cluster registry. This option is a no-op on 'up'.
-h, --help help for run
--include strings Path to Kubernetes resource manifests, ex. Role, Subscription. These supplement or override defaults generated by up/down
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
--include strings Path to Kubernetes resource manifests, ex. Role, Subscription. These supplement or override defaults generated by up/down
--include strings Path to Kubernetes resource manifests, ex. Role, Subscription. These supplement or override defaults generated by run/cleanup

Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

After solving the small nits added by @joelanford
It shows ok to b merged for me as well.

/lgtm
/approved

@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2020
@estroz estroz merged commit f5f80cb into operator-framework:master Jan 15, 2020
@estroz estroz deleted the feature/olm-poc-cli branch January 15, 2020 21:35
joelanford added a commit to joelanford/operator-sdk that referenced this pull request Jan 17, 2020
joelanford added a commit to joelanford/operator-sdk that referenced this pull request Jan 17, 2020
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 size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants