Skip to content
This repository has been archived by the owner on Oct 10, 2023. It is now read-only.

Bump controller-runtime to v0.9.0 #445

Closed
wants to merge 1 commit into from

Conversation

jpmcb
Copy link
Contributor

@jpmcb jpmcb commented Aug 17, 2021

What this PR does / why we need it:
Fixes tanzu-framework packages not being "go get-able". From a new go module, in a new vm, the following error occurs:

$ go get github.com/vmware-tanzu/tanzu-framework/cmd/cli/tanzu@main
...
# sigs.k8s.io/controller-runtime/pkg/metrics
../go/pkg/mod/sigs.k8s.io/controller-runtime@v0.7.0/pkg/metrics/client_go_adapter.go:134:3: cannot use &latencyAdapter{...} (type *latencyAdapter) as type metrics.LatencyMetric in field value:
        *latencyAdapter does not implement metrics.LatencyMetric (wrong type for Observe method)
                have Observe(string, url.URL, time.Duration)
                want Observe(context.Context, string, url.URL, time.Duration)
../go/pkg/mod/sigs.k8s.io/controller-runtime@v0.7.0/pkg/metrics/client_go_adapter.go:135:3: cannot use &resultAdapter{...} (type *resultAdapter) as type metrics.ResultMetric in field value:
        *resultAdapter does not implement metrics.ResultMetric (wrong type for Increment method)
                have Increment(string, string, string)
                want Increment(context.Context, string, string, string)

This is documented in #444

With this PR, bumps controller-runtime to v0.9.0 which resolves dependencies with client-go v0.21 correctly. But, this keeps the various replace statements for both dependencies so there is no functional change, but now, will resolve dependencies correctly so the packages are in a proper state to be pulled.

Which issue(s) this PR fixes:
Fixes #444

Describe testing done for PR:
make test passes and I am able to locally pull tanzu framework again. We should validate this works once it is ontop of main

Does this PR introduce a user-facing change?:

NONE

New PR Checklist

  • Ensure PR contains only public links or terms
  • Use good commit messages
  • Squash the commits in this branch before merge to preserve our git history
  • If this PR is just an idea or POC, use a Draft PR instead of a full PR
  • Add appropriate kind label according to what type of issue is being addressed.

Signed-off-by: John McBride <jmcbride@vmware.com>
@jpmcb jpmcb requested review from danniel1205, shyaamsn and a team as code owners August 17, 2021 14:51
@jpmcb jpmcb added area/repo maintenance kind/bug PR/Issue related to a bug TCE labels Aug 17, 2021
Copy link
Contributor

@iancoffey iancoffey left a comment

Choose a reason for hiding this comment

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

lgtm thank you for this fix, I also am seeing this issue. I do wonder though how we got to this bad situation wrt C-R versions, as some of our deps relied on 0.7.0.

We will have to determine how and when this update became necessary before merging, so I will hold off on adding the ok-to-merge label. cc @vuil

@jpmcb
Copy link
Contributor Author

jpmcb commented Aug 17, 2021

as some of our deps relied on 0.7.0

I didn't change the replace statement in the top level go.mod but I did add a new replace for controller-runtime for the module in addons/.

So, ultimately, there shouldn't be any functional change as the dependencies ultimately still resolve down to the original v0.5.14 in the top level go module and v0.7.0 in the addons/ go module. But I believe that since modules are "replaced" last in the dependency graph, the versions still need to be compatible. Thus, the bump to v0.9.0 of C-R

@vijaykatam
Copy link
Contributor

I think you need to bump cluster-api to 0.4.x to be able to use controller-runtime 0.9.x, see kubernetes-sigs/cluster-api@da54aae.

@vijaykatam vijaykatam added the do-not-merge/hold Some fixes necessary, hold for merging label Aug 17, 2021
@vijaykatam
Copy link
Contributor

@jpmcb can this be closed? There are other PRs in flight to bump controller-runtime as part of CAPI bump.

@jpmcb
Copy link
Contributor Author

jpmcb commented Oct 21, 2021

@vijaykatam - yup! Makes sense: this was more just a signal & workaround for #444 so if this is being tackled elsewhere, sounds good

@jpmcb jpmcb closed this Oct 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/repo maintenance do-not-merge/hold Some fixes necessary, hold for merging kind/bug PR/Issue related to a bug TCE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to pull cli/cmd/tanzu package via go get
4 participants