Bump operator-registry dependency by kevinrizza · Pull Request #2662 · operator-framework/operator-sdk

kevinrizza · 2020-03-16T17:48:41Z

Description of the change:

*Update operator-registry dependency to 1.6.0
*Adds fix for generated dockerfiles
*Adds optional output-directory parameter

Motivation for the change:

Include fixes for operator-sdk bundle create

Bowenislandsong · 2020-03-16T17:50:51Z

/lgtm

camilamacedo86

See that it is not passing in the CI because the go.mod and .go.sum are not updated properly.
See; https://travis-ci.org/github/operator-framework/operator-sdk/jobs/663172166#L969
Note that you need to update the testdata framework too .

So, I'd recommend to you:

Run go mod tidy
Then, you need to exec the script ./hack/generate/gen-test-framework.sh to update it.

cmd/operator-sdk/bundle/create.go

camilamacedo86 · 2020-03-16T19:32:15Z

CHANGELOG.md

 - Upgrade the Helm dependency version from `v3.0.1` to `v3.0.2`. ([#2621](https://github.com/operator-framework/operator-sdk/pull/2621)) 
 - Changed the scaffolded `serveCRMetrics` to use the namespaces informed in the environment variable `WATCH_NAMESPACE` in the MultiNamespace scenario. ([#2603](https://github.com/operator-framework/operator-sdk/pull/2603)) 
 - Improve skip metrics logs when running the operator locally in order to make clear the information for Helm based operators. ([#2603](https://github.com/operator-framework/operator-sdk/pull/2603))
+- Bumped operator-registry dependency version to `v1.6.0` to update `bundle create` behavior. Includes adding new optional `output-dir` flag to specify a result bundle directory.


Could you please update it as follows?

In the changes section:

- Upgrade the `operator-registry` dependency version from ` v1.5.7`to `v1.6.0` to update `bundle create` behaviour. ([#2662](https://github.com/operator-framework/operator-sdk/pull/2662))

In the add section:

- Add the new flag `output-dir` to the command `operator-sdk bundle create`. ([#2662](https://github.com/operator-framework/operator-sdk/pull/2662))

camilamacedo86

I did not test it locally, would really be great we have tests implemented for it.
But it shows ok for me. @hasbro17 @estroz, wdyt?

/lgtm

estroz · 2020-03-18T17:20:39Z

cmd/operator-sdk/bundle/create.go

@@ -96,7 +96,7 @@ $ operator-sdk bundle create \
 				defer cleanup()


These cleanup funcs assume the generated files are present in c.directory. You'll have to set the correct cleanup dir in c.cleanupFuncs():

func (c bundleCmd) cleanupFuncs() (fs []func()) { dir := c.outputDir if dir == "" { dir = c.directory } metaDir := filepath.Join(dir, bundle.MetadataDir) dockerFile := filepath.Join(dir, bundle.DockerFile) ... }

@estroz ah. so, the explicit purpose of --output-dir is to end up with a generated directory as output so that you don't have to keep generating them everytime you build. annotations.yaml is sort of supposed to be a long lived file that you can edit yourself manually.

It seems like --generate-only and --output-dir being set are either interchangeable (but with a specific dir set) OR passing the output-dir while also building would result in the files living around after you build.

IMO though it seems weird to me to generate and throw these files away all the time, given that OLM's packaging format is at some point probably going to want you to drive other stuff in the metadata/ folder (ex. https://github.com/operator-framework/enhancements/blob/master/enhancements/operator-dependency-resolution.md#dependency-specification). I had assumed that we treated the bundle command as a "run once to scaffold things" rather than something that should default to deleting stuff afterwards. Maybe we should sync up on this?

Also this change renames the index.Dockerfile and throws it in the same working directory as the context of where you're running the command. so if we end up wanting to clean it up, we will need to delete that file too.

jmrodri · 2020-03-20T19:42:54Z

@kevinrizza this is still waiting on @estroz comment to be addressed: https://github.com/operator-framework/operator-sdk/pull/2662/files#r394514725

openshift-ci-robot · 2020-03-23T17:39:11Z

New changes are detected. LGTM label has been removed.

camilamacedo86 · 2020-03-23T17:41:07Z

CHANGELOG.md

 - Upgrade the Helm dependency version from `v3.0.1` to `v3.0.2`. ([#2621](https://github.com/operator-framework/operator-sdk/pull/2621)) 
 - Changed the scaffolded `serveCRMetrics` to use the namespaces informed in the environment variable `WATCH_NAMESPACE` in the MultiNamespace scenario. ([#2603](https://github.com/operator-framework/operator-sdk/pull/2603)) 
 - Improve skip metrics logs when running the operator locally in order to make clear the information for Helm based operators. ([#2603](https://github.com/operator-framework/operator-sdk/pull/2603))
+- Upgrade the `operator-registry` dependency version from `v1.5.7`to `v1.6.0` to update `bundle create` behaviour. ([#2662](https://github.com/operator-framework/operator-sdk/pull/2662))


Could we not bump v1.6.1 already?

We could but that version is a kube version bump and doesn't directly affect the registry dependency at all. I would prefer to generate a followup pr for that, especially given the context of this PR and the purpose of the discussion around it

kevinrizza · 2020-03-23T17:51:05Z

@jmrodri @estroz updated, ptal

jmrodri · 2020-03-24T19:23:32Z

@kevinrizza you've been hit by the CHANGELOG.md conflict :( please rebase

*Update operator-registry dependency to 1.6.0 *Adds fix for generated dockerfiles *Adds optional output-directory parameter *Update cleanup funcs to handle new structure

kevinrizza · 2020-03-24T19:28:32Z

Rebased, ptal :)

estroz · 2020-03-24T21:32:21Z

cmd/operator-sdk/bundle/cmd.go

+	if manifestDir == "" {
+		manifestDir = c.directory
+	}
+	absManifestDir, _ := filepath.Abs(manifestDir)


Does this need to be abs? If so import log "github.com/sirupsen/logrus" above and add:

Suggested change

absManifestDir, _ := filepath.Abs(manifestDir)

absManifestDir, err := filepath.Abs(manifestDir)

if err != nil {

log.Fatal(err)

}

estroz · 2020-03-24T21:32:46Z

cmd/operator-sdk/bundle/cmd.go

+	metaDir := filepath.Join(manifestParent, bundle.MetadataDir)
 	metaExists := isExist(metaDir)
+
+	workingDir, _ := os.Getwd()


Suggested change

workingDir, _ := os.Getwd()

workingDir, err := os.Getwd()

if err != nil {

log.Fatal(err)

}

kevinrizza · 2020-03-25T13:13:40Z

Closing in favor of #2714 which includes the commit in this pr

openshift-ci-robot requested review from fabianvf and hasbro17 March 16, 2020 17:48

openshift-ci-robot assigned Bowenislandsong Mar 16, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 16, 2020

camilamacedo86 reviewed Mar 16, 2020

View reviewed changes

cmd/operator-sdk/bundle/create.go Show resolved Hide resolved

kevinrizza force-pushed the update-operator-bundle-generate branch from 5a41d35 to da66ed5 Compare March 16, 2020 19:20

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 16, 2020

camilamacedo86 reviewed Mar 16, 2020

View reviewed changes

kevinrizza force-pushed the update-operator-bundle-generate branch from da66ed5 to cd72770 Compare March 16, 2020 19:37

camilamacedo86 approved these changes Mar 16, 2020

View reviewed changes

openshift-ci-robot assigned camilamacedo86 Mar 16, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 16, 2020

camilamacedo86 requested a review from joelanford March 17, 2020 09:38

estroz suggested changes Mar 18, 2020

View reviewed changes

kevinrizza force-pushed the update-operator-bundle-generate branch from cd72770 to d1e7009 Compare March 23, 2020 17:39

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm Indicates that a PR is ready to be merged. labels Mar 23, 2020

camilamacedo86 reviewed Mar 23, 2020

View reviewed changes

kevinrizza force-pushed the update-operator-bundle-generate branch from d1e7009 to 3bbc7cd Compare March 23, 2020 17:45

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 23, 2020

Bump operator-registry dependency

cbcdc13

*Update operator-registry dependency to 1.6.0 *Adds fix for generated dockerfiles *Adds optional output-directory parameter *Update cleanup funcs to handle new structure

kevinrizza force-pushed the update-operator-bundle-generate branch from 3bbc7cd to cbcdc13 Compare March 24, 2020 19:27

estroz suggested changes Mar 24, 2020

View reviewed changes

estroz mentioned this pull request Mar 25, 2020

[WIP] bundle: add subcommand to build operator index images #2714

Closed

3 tasks

kevinrizza closed this Mar 25, 2020

This was referenced Mar 25, 2020

[WIP] bundle: fix bundle create, bump operator-registry v1.6.0 #2715

Closed

bundle validate: validate a bundle directory #2737

Merged

		@@ -96,7 +96,7 @@ $ operator-sdk bundle create \
		defer cleanup()

-	absManifestDir, _ := filepath.Abs(manifestDir)
+	absManifestDir, err := filepath.Abs(manifestDir)
+	if err != nil {
+		log.Fatal(err)
+	}

Conversation

kevinrizza commented Mar 16, 2020

Uh oh!

Bowenislandsong commented Mar 16, 2020

Uh oh!

camilamacedo86 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmrodri commented Mar 20, 2020

Uh oh!

openshift-ci-robot commented Mar 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinrizza commented Mar 23, 2020

Uh oh!

jmrodri commented Mar 24, 2020

Uh oh!

kevinrizza commented Mar 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinrizza commented Mar 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants