Skip to content

generate: add bundle subcommand for current project layouts#3088

Merged
estroz merged 1 commit intooperator-framework:masterfrom
estroz:feature/generate-bundle-legacy
May 23, 2020
Merged

generate: add bundle subcommand for current project layouts#3088
estroz merged 1 commit intooperator-framework:masterfrom
estroz:feature/generate-bundle-legacy

Conversation

@estroz
Copy link
Copy Markdown
Member

@estroz estroz commented May 22, 2020

Description of the change:

  • cmd/operator-sdk/generate/bundle: add NewCmdLegacy that returns a bundle subcommand configured to generate bundles for current project layouts.
  • internal/generate/clusterserviceversion: add GenerateLegacy and LegacyOption to generate CSVs for legacy projects.

Motivation for the change: current project layouts should have a generate bundle command to differentiate between bundle and package manifests formats.

/cc @hasbro17 @jmrodri @camilamacedo86

/kind feature

@openshift-ci-robot openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label May 22, 2020
@estroz
Copy link
Copy Markdown
Member Author

estroz commented May 22, 2020

Waiting on #3087, still good to review though.

Comment thread cmd/operator-sdk/generate/bundle/cmd.go
Comment thread cmd/operator-sdk/generate/bundle/bundle_legacy.go
Comment thread cmd/operator-sdk/generate/bundle/bundle.go
@@ -0,0 +1,131 @@
// Copyright 2020 The Operator-SDK Authors
Copy link
Copy Markdown
Contributor

@camilamacedo86 camilamacedo86 May 22, 2020

Choose a reason for hiding this comment

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

Why do we need to create the legacy one? Is not it == the new one and just change the paths?
Is not harder to have the same implementation duplicated instead of having the code centralized to keep it maintained? WDYT? See: #2948 (comment)

PS.; My first approach was to do it as you did here, however, after some comments from @joelanford I think it makes more sense just change the path instead of it as he suggested.

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.

I'm of the opinion that some code duplication is OK, especially with complex command setup like this. It's easier to maintain, then remove when we remove current project layout support.

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.

I'm okay with duplication in this case as well. From my experience with trying to reuse the code for generate csv to be used by both the old and new CLI commands it made the underlying command a lot more convoluted.

@camilamacedo86 operator-sdk run local was a much simpler command where we just had to change 1 path.

@estroz estroz force-pushed the feature/generate-bundle-legacy branch 2 times, most recently from 717f474 to 0a5d435 Compare May 22, 2020 19:10
@hasbro17
Copy link
Copy Markdown
Contributor

We need a CHANGELOG fragment for adding this cmd to the existing CLI right?

Copy link
Copy Markdown
Contributor

@hasbro17 hasbro17 left a comment

Choose a reason for hiding this comment

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

We'll need to add a CHANGELOG fragment but LGTM otherwise.

@estroz estroz force-pushed the feature/generate-bundle-legacy branch from 0a5d435 to 33535e9 Compare May 23, 2020 01:03
@estroz estroz merged commit 6c48dc2 into operator-framework:master May 23, 2020
@estroz estroz deleted the feature/generate-bundle-legacy branch May 23, 2020 01:34
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.

4 participants