-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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: bundle build
builds operator bundle images
#2076
Conversation
1. `bundle generate` command to generate `annotations.yaml` metadata from bundle manifests 2. `bundle build` command to build build manifests image from local bundle manifests. In the background, the `generate` command will be run as a part of `build` command to generate `annotations.yaml` metadata. This bundle commands will use bundle library from operator-registry repository. Signed-off-by: Vu Dinh <vdinh@redhat.com>
Hi @dinhxuanvu, Thank you for your contribution. 🥇 I'd like to share that IMHO we should do it using the current operator-sdk olm commands instead of changing the build one and in the current structure used by SDK. See my comments here #1913 in order to make it clear. However, please wait to see what the others in the team think about it before doing changes. They may have another point of view over it. I will just let my comments in order to share/raise it. c/c @joelanford |
cmd/operator-sdk/bundle/build.go
Outdated
// newBundleBuildCmd returns a command that will build operator bundle image. | ||
func newBundleBuildCmd() *cobra.Command { | ||
bundleBuildCmd := &cobra.Command{ | ||
Use: "build", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be something as operator-sdk olm-catalog gen-csv --csv-version 0.1.0 --create-bundle
Following why I think it.
- The --csv-version 0.1.0 is required to know the version which should be used to create the bundle
- I understand that the CSV is part of the bundle according to the info in https://operatorhub.io/bundle and https://github.com/operator-framework/community-operators
- The operator-SDK has already a structure to generate the olm-catalogue files then in POV would be easier/better to keep it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Camila,
Thank you for the initial review. I would like to add the bundle enhancement and bundle library docs so you can take a look and have a better understanding on what my team is trying to do with this PR. I should've added those docs on commit message but I forgot. My apology.
One note that the PR for adding bundle package to operator-registry is still pending-merge so that's why this PR fails some tests due to the package is not available yet.
The goal of this PR to add a new bundle
command under the top-level operator-sdk
command. The bundle
command itself has two subcommands to generate
bundle metadata and build
bundle container image. It should strictly use bundle package/library to perform its tasks without interfering with any existing functionalities of SDK. This PR shouldn't change or impact any existing SDK commands/subcommands. In a way, this is a "plug-in" that is independent in some way. I do not wish to nest/add/wrap this bundle command tool under other existing SDK commands as it is a brand-new feature and if we do so, it will affect other existing commands which I believe something Joe doesn't want. Secondly, bundle command requires a few flags that if nested under other commands will create very messy UX as the commands will be too longer and confusing especially if the command contains something that has nothing to do with creating bundle image such as olm-catalog
or even olm
itself. OLM is supposed to be a consumer of bundle images, not creator.
If you have any further questions, please feel free to ask. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @dinhxuanvu,
Thank you for share the info. Following my main concerns/review on it.
IMHO
- it should follow up the same idea/standard used in the other SDK commands
- it should by default use the SDK structure project and the flags are optional instead of mandatory
- It is not passing in the tests
- it is missing docs, tests, changelog
- it shows missing the impl of each command.
8e93155
to
03530ea
Compare
cmd/operator-sdk/bundle/build.go
Outdated
After the build process is completed, a container image would be built | ||
locally in docker and available to push to a container registry. | ||
|
||
$ operator-sdk bundle build -d /test/ -t quay.io/example/operator:v0.1.0 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here is with the short flags values when down it is with the full flag values. IMHO all should follow the same standard. I'd keep the full flag in order to make clear.
cmd/operator-sdk/bundle/generate.go
Outdated
bundleGenerateCmd := &cobra.Command{ | ||
Use: "generate", | ||
Short: "Generate operator bundle metadata and Dockerfile", | ||
Long: `The "operator-sdk bundle generate" command will generate operator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is required a operator-sdk bundle build
and a operator-sdk bundle generate
one?
What are the diff between both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dinhxuanvu based on the current command descriptions, a user wouldn't necessarily be able to tell the difference between the commands. Can you specify that generate
only creates the operator metadata without building the image, and that a non-default builder can be used to build the metadata image?
Additional flags are added to include package and channels information into the annotations.yaml. Plus, several optional flags for different use case options. Signed-off-by: Vu Dinh <vdinh@redhat.com>
03530ea
to
b700d51
Compare
@dinhxuanvu: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not passing in the tests and missing the common requirements: #2076 (comment)
cmd/operator-sdk/bundle/build.go
Outdated
"github.com/spf13/cobra" | ||
) | ||
|
||
var ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This command would be easier to manage by having these variables be struct fields:
type bundleCmd struct {
dirBuildArgs string
tagBuildArgs string
imageBuilderArgs string
packageNameArgs string
channelsArgs string
channelDefaultArgs string
overwriteArgs bool
}
func newBundleBuildCmd() *cobra.Command {
c := bundleCmd{}
bundleBuildCmd := &cobra.Command{
...
RunE: func(cmd *cobra.Command, args []string) error {
return bundle.BuildFunc(c.dirBuildArgs, c.tagBuildArgs, c.imageBuilderArgs,
c.packageNameArgs, c.channelsArgs, c.channelDefaultArgs, c.overwriteArgs)
}
}
bundleBuildCmd.Flags().StringVarP(&c.dirBuildArgs, "directory", "d", "", "The directory where bundle manifests are located")
...
}
Since generate
args are a subset of build
's, this struct can be shared between commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This command should be put under the alpha
subcommand since it is a new feature that may change in the near future.
CLI wrappers for operator-registry bundle image functions
cmd.AddCommand( | ||
olm.NewCmd(), | ||
kubebuilder.NewCmd(), | ||
bundle.NewCmd(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to pretty quickly decide where to put the OLM integration subcommands in the CLI. I think alpha
is probably okay for this PR, but we'll need a follow-up on the heels of this to promote these OLM alpha
subcommands.
Hi @dinhxuanvu, Really tks for your contribution. 🥇 I just add a few comments and I'd like to highlight that it is not passing in the CI. |
Hi @dinhxuanvu, It still not passing see:
|
@camilamacedo86 Hey Camila. Eric is taking over this PR and currently making changes to align with what SDK team plans to do with the bundle library integration. So he is in charge of this PR now. I will help out with the review and making changes to bundle library on operator-registry repo if needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shows fine for me. 👍
Great contribution 🥇
/lgtm
/apprroved
PS.: It needs to be checked by @joelanford and/or @estroz before be merged.
New changes are detected. LGTM label has been removed. |
bundle build
builds operator bundle images
Description of the change:
bundle build
command to build build manifests image from local bundle manifests.This bundle commands will use bundle library from operator-registry repository.
Motivation for the change:
Enable operator-sdk to build bundle manifests images using bundle library from
operator-registry repository.