Skip to content

Bump deps to version 1.14#178

Closed
rcarrillocruz wants to merge 1 commit intoopenshift:masterfrom
rcarrillocruz:bump_to_114
Closed

Bump deps to version 1.14#178
rcarrillocruz wants to merge 1 commit intoopenshift:masterfrom
rcarrillocruz:bump_to_114

Conversation

@rcarrillocruz
Copy link
Copy Markdown
Contributor

@rcarrillocruz rcarrillocruz commented May 24, 2019

No description provided.

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 24, 2019
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

openshift-ci-robot commented May 24, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rcarrillocruz
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: danwinship

If they are not already assigned, you can assign the PR to them by writing /assign @danwinship in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@squeed
Copy link
Copy Markdown
Contributor

squeed commented May 24, 2019

That was fast :-)

@squeed squeed requested review from squeed and removed request for knobunc and smarterclayton May 24, 2019 16:26
@rcarrillocruz rcarrillocruz changed the title Bump deps to version 1.14 WIP Bump deps to version 1.14 May 24, 2019
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 24, 2019
@rcarrillocruz
Copy link
Copy Markdown
Contributor Author

rcarrillocruz commented May 27, 2019

/retest

@rcarrillocruz rcarrillocruz force-pushed the bump_to_114 branch 2 times, most recently from a76a6b2 to 3a473c0 Compare May 27, 2019 11:47
@rcarrillocruz rcarrillocruz changed the title WIP Bump deps to version 1.14 Bump deps to version 1.14 May 27, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 27, 2019
@squeed
Copy link
Copy Markdown
Contributor

squeed commented May 27, 2019

While you're at it, want to bump operator-sdk to 0.8? Annoyingly, you will have to figure out any changes to main.go from the operator-sdk templating.

@rcarrillocruz
Copy link
Copy Markdown
Contributor Author

rcarrillocruz commented May 27, 2019

While you're at it, want to bump operator-sdk to 0.8? Annoyingly, you will have to figure out any changes to main.go from the operator-sdk templating.

Yah, can look at it.

@rcarrillocruz
Copy link
Copy Markdown
Contributor Author

rcarrillocruz commented May 27, 2019

/retest

squeed
squeed reviewed May 28, 2019
View reviewed changes
Comment thread Gopkg.toml
[[override]]
name = "sigs.k8s.io/controller-runtime"
version = "=v0.1.8"
version = "=v0.2.0-beta.1"
Copy link
Copy Markdown
Contributor

@squeed squeed May 28, 2019

Choose a reason for hiding this comment

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

don't bump this yet, this version is definitely not ready to go.

Copy link
Copy Markdown
Contributor Author

@rcarrillocruz rcarrillocruz May 28, 2019

Choose a reason for hiding this comment

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

So yeah, this tripped me up.
The issue is that 0.2.0-beta.1 is the tag for controller-runtime to bump to Kubernetes 1.14.
Without pinning to that, we hit this on compile time:

# github.com/openshift/cluster-network-operator/vendor/sigs.k8s.io/controller-runtime/pkg/leaderelection
vendor/sigs.k8s.io/controller-runtime/pkg/leaderelection/leader_election.go:83:25: not enough arguments in call to resourcelock.New
        have (string, string, string, "github.com/openshift/cluster-network-operator/vendor/k8s.io/client-go/kubernetes/typed/core/v1".CoreV1Interface, resourcelock.ResourceLockConfig)
        want (string, string, string, "github.com/openshift/cluster-network-operator/vendor/k8s.io/client-go/kubernetes/typed/core/v1".CoreV1Interface, "github.com/openshift/cluster-network-operator/vendor/k8s.io/client-go/kubernetes/typed/coordination/v1".CoordinationV1Interface, resourcelock.ResourceLockConfig)

Check pkg/leaderelection/leader_election.go.
kubernetes-sigs/controller-runtime@71cf6f6#diff-2590281733c8f856c7a8fb319d5d3cc5R87

So I wonder if we should wait for controller-runtime to release a version that is non-beta, then we change the pin.
Reverting to 0.1.8 is a no-go.

Copy link
Copy Markdown
Contributor

@squeed squeed May 28, 2019

Choose a reason for hiding this comment

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

Ugh. @lilic any suggestions?

Copy link
Copy Markdown

@lilic lilic May 29, 2019

Choose a reason for hiding this comment

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

Yes, we have a PR out in operator-sdk to switch to controller-runtime 0.2.0-x, we haven't merged it yet as they are still in beta (from what I spoke with the maintainer they should be cutting the stable release soon if no bugs are found in testing). There are a lot of changes there, you can see in operator-framework/operator-sdk#1388 PR if that is of any help. Sadly they did not release an intermediate version with bumping k8s to 1.14.

I would agree to the following as we have things ready:

So I wonder if we should wait for controller-runtime to release a version that is non-beta, then we change the pin.

@rcarrillocruz
Copy link
Copy Markdown
Contributor Author

rcarrillocruz commented May 29, 2019

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 29, 2019
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

openshift-ci-robot commented Jun 18, 2019

@rcarrillocruz: PR needs rebase.

Details

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.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 18, 2019
@lilic
Copy link
Copy Markdown

lilic commented Jul 4, 2019

hi @rcarrillocruz so we merged the bump to k8s 1.14 into a branch on operator-sdk. You can see the PR operator-framework/operator-sdk#1512. Maybe start testing against that commit from https://github.com/operator-framework/operator-sdk/tree/refactor/controller-runtime-v0.2.0, in this PR. We are blocked on controller-runtime cutting the final release of v0.2.0. But things should not change too much when we do merge that branch into operator-sdk master.

@rcarrillocruz
Copy link
Copy Markdown
Contributor Author

rcarrillocruz commented Jul 4, 2019

oki, thx for the update @lilic

@squeed squeed closed this Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@squeed squeed squeed left review comments

@lilic lilic lilic left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rcarrillocruz @openshift-ci-robot @squeed @lilic