Skip to content

cli (new): add run packagemanifests#3253

Merged
estroz merged 1 commit intooperator-framework:masterfrom
estroz:feature/run-packagemanifests
Jun 30, 2020
Merged

cli (new): add run packagemanifests#3253
estroz merged 1 commit intooperator-framework:masterfrom
estroz:feature/run-packagemanifests

Conversation

@estroz
Copy link
Copy Markdown
Member

@estroz estroz commented Jun 17, 2020

Description of the change:

  • cmd/operator-sdk: add run packagemanifests to new CLI, and set defaults for new project layouts
  • test/e2e-new: test run packagemanifests
  • test/utils: add utils to install/uninstall OLM

Motivation for the change: run packagemanifests should be available for new operator project layouts. This command must be used on operator projects with a package manifests format available.

/kind feature

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. labels Jun 17, 2020
@estroz estroz force-pushed the feature/run-packagemanifests branch from 2c6f22b to a9c34f8 Compare June 18, 2020 03:13
@estroz
Copy link
Copy Markdown
Member Author

estroz commented Jun 18, 2020

This command will incidentally read config/packagemanifests/bases and assume it is a valid package, causing this command to error. #3257 fixes this as it moves the relevant package manifests files to a directory <project root>/packagemanifests/ separate from kustomize-related files.

@estroz estroz force-pushed the feature/run-packagemanifests branch 6 times, most recently from c731f6a to 581ad00 Compare June 22, 2020 16:41
@estroz estroz marked this pull request as ready for review June 22, 2020 16:44
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 22, 2020
Comment thread cmd/operator-sdk/run/packagemanifests/packagemanifests.go Outdated
Comment thread hack/tests/e2e-go-new.sh Outdated
Comment thread internal/olm/operator/packagemanifests.go Outdated
Comment thread test/utils/utils.go
@estroz estroz force-pushed the feature/run-packagemanifests branch 3 times, most recently from 21d4aa1 to f2d0b07 Compare June 22, 2020 20:53
…uts.

This command must be used on operator projects with a package manifests
format available.

test/e2e-new: test `run packagemanifests`

test/utils: add utils to install/uninstall OLM
@estroz estroz force-pushed the feature/run-packagemanifests branch from f2d0b07 to 68d194f Compare June 22, 2020 20:56
@estroz estroz requested a review from joelanford June 22, 2020 21:31
Use: "packagemanifests",
Short: "Run an Operator organized in the package manifests format with OLM",
Short: "Deploy an Operator in the package manifests format with OLM",
Long: `'run packagemanifests' deploys an Operator's package manifests with OLM. The command's argument
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if we have a choice between run packagemanifests and run olm, I'd vote for the latter one.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@tengqm the reason we're thinking of using packagemanifests is to distinguish it from an upcoming run bundle subcommand. The idea is that we want to support various OLM packaging formats for the run subcommand, and if we tried to encapsulate all of that under a single run olm command, the UX would get unwieldy since there are some differences in how they work.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the info, @joelanford. I am just not a big fan of long commands, :).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@estroz Perhaps we could add a cobra alias for this (e.g. operator-sdk run pm)?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

SGTM, I'll make a follow-up.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
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 we decide whether or not to add an alias.

@estroz estroz merged commit cb8bf47 into operator-framework:master Jun 30, 2020
@estroz estroz deleted the feature/run-packagemanifests branch June 30, 2020 16:08
olm.NewCmd(),
printdeps.NewCmd(),
run.NewCmd(),
run.NewCmdLegacy(),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

helm/Ansible still requiring run operator-sdk run --local with the new layout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/feature Categorizes issue or PR as related to a new feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants