upgrade kb and controller-runtime dep version

[camilamacedo86]

Description of the change:

• Upgrade kb commit from f7a3b65dd250 to c993a2a221fe
• Upgrade controller-runtime version from v0.6.2 to v0.6.3. More info: https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.6.3

Motivation for the change:

• Address bugfixes done in Kubebuilder so far
• Solve tech-debts
• Keep the projects aligned.

Checklist

If the pull request includes user-facing changes, extra documentation is required:

• Add a new changelog fragment in changelog/fragments (see changelog/fragments/00-template.yaml)
• Add or update relevant sections of the docs website in website/content/en/docs

[estroz]

I think we need to have a discussion about how the changelog refers to upstream bugfixes that the SDK receives through dep bumps.

My opinion: we shouldn't be making changelog entries for upstream bug fixes unless there is an issue filed in the SDK repo (unless it is serious enough to precipitate here), or if a migration is needed. All of these bugs and their fixes are already documented upstream. If we decide to document bugfixes in the SDK changelog, then we can't be selective about which ones we document and must document all bugfixes.

/cc @jmrodri @joelanford

[joelanford]

I generally agree with @estroz, but I would propose a perhaps reasonable middle ground: If we make a dependency bump that will have user-facing changes, we have a simple changelog entry that says something along the lines of

Bumped dependency A to vX.Y.Z. See <link> for details.

Where <link> is a link to the github release or changelog notes or whatever other doc documents that release.

[camilamacedo86]

Hi @joelanford,

I generally agree with @estroz, but I would propose a perhaps reasonable middle ground: If we make a dependency bump that will have user-facing changes, we have a simple changelog entry that says something along the lines of

Bumped dependency A to vX.Y.Z. See for details.

Where is a link to the github release or changelog notes or whatever other doc documents that release.

Currently, we cannot do that because we still bumping the commits which indeed is my motivation here. Note that after we do a kb release and start to have more actively releases from there we will be able to achieve the goal in this way. Now, it is better we keep both aligned as much as possible otherwise will be hard we know what changes from the commit X to the commit Y.

[joelanford]

Ah I see. Since we're currently pinning to a random commit on master, there isn't a pretty release page or changelog. Could we link instead to a commit range? I think GitHub handles that nicely.

[estroz]

Operator projects don't directly depend on kubebuilder, so this is rather confusing. We probably don't need to notify users about operator-sdk binary dependency changes (those that are not scaffolded) unless they affect commands that have a runtime (ex. k8s versions).

How about

Suggested change

	Upgrade the Kubebuilder CLI dependecy version from `f7a3b65dd250` to `c993a2a221fe`.
	Fix scaffolded (multi-group only) `controllers/<group>/suite_test.go` to have
	a correct `envtest.Environment.CRDDirectoryPaths` ([kubebuilder#1665](https://github.com/kubernetes-sigs/kubebuilder/pull/1665)).

[camilamacedo86]

Then, we came back for the first version of this PR where all changes/bugfixes that impact the end-user was described on it. see: 59cb168

Note that it was a suggestion made by @joelanford

[estroz]

Do you think users care about what version of kubebuilder the operator-sdk binary uses, or whether a scaffold fix occurred? You can still link the commit range without discussing it.

[camilamacedo86]

Do you think users care about what version of kubebuilder the operator-sdk binary uses, or whether a scaffold fix occurred?

I do not think so. Because of this my first version of this PR just add fragment for all bugfixes changes that are visible to the SDK end-users

You can still link the commit range without discussing it.

What is your suggestion? How do you think that we can do that?
I am totally open mind here. I just try to avoid back an forwards as for example, are we ok to come back it to the original idea? Or what are your thoughts?

[camilamacedo86]

I applied your suggestion specifically. I am OK with too.

[joelanford]

@camilamacedo86 It looks like you ran make bindata, which caused unnecessary/unrelated changes to internal/bindata/olm/manifests-0.15.1.go. Can you revert that change? I think running make fix will resolve it since it looks like a go fmt issue.

Also @varshaprasad96 @estroz we may want to use this fork: https://github.com/kevinburke/go-bindata

It seems to have more frequent maintenance, is the fork used for brew install go-bindata, and has binary releases which would enable us to consolidate the download and version checks in the new tools/scripts/fetch script.

One of the improvements is that it automatically go fmts the output it produces which I think would have resolved the error that @camilamacedo86 has with the changes to this file in this PR.

[camilamacedo86]

Hi @joelanford,

@camilamacedo86 It looks like you ran make bindata, which caused unnecessary/unrelated changes to internal/bindata/olm/manifests-0.15.1.go. Can you revert that change? I think running make fix will resolve it since it looks like a go fmt issue.

We cannot use the go fmt in the bindata. See: https://travis-ci.org/github/operator-framework/operator-sdk/jobs/737366995#L1335-L1342.

IHMO

• the make generate cannot call the target to gen the bindata.
• we need to ignore the bindata in the lint
• the govet and gofmt ought to be checked via lint instead of shell.

[joelanford]

@camilamacedo86 Why does this PR have any changes to internal/bindata/olm/manifests-0.15.1.go?

the make generate cannot call the target to gen the bindata.

Neither make generate nor make test-sanity call make bindata and make bindata is not called in CI.

we need to ignore the bindata in the lint

I disagree, we should make sure all of our committed code is formatted correctly with go fmt

the govet and gofmt ought to be checked via lint instead of shell.

Don't think this is related to the issue.

[joelanford]

These lines appear to be caused by running make bindata and should be reverted.

@varshaprasad96 we should update the bindata generation script to make sure it does not cause side effects like this.

[camilamacedo86]

It is the go.mod of the project.
See that it still as it is and passing in the CI after we revert the changes made via the make fix on the bindata.

[camilamacedo86]

However, I reverted that I executed go mod tidy.

[estroz]

/lgtm

[camilamacedo86]

@joelanford since our discussions here were only over the fragment and all suggestions made were addressed it shows fine to get merged. if you wish, we can change some word in a follow up as well.