Skip to content
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

✨ Bump dependencies for Kubernetes 1.15.4 #599

Merged
merged 6 commits into from
Oct 10, 2019

Conversation

joelanford
Copy link
Member

@joelanford joelanford commented Sep 11, 2019

Following up on #495, this bumps Kubernetes dependencies to 1.15.4.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 11, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @joelanford. Thanks for your PR.

I'm waiting for a kubernetes-sigs or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 11, 2019
@mengqiy
Copy link
Member

mengqiy commented Sep 16, 2019

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 16, 2019
@alvaroaleman
Copy link
Member

At this point it probably makes more sense to wait for Kube 1.16 and then use that, James already created a PR for that: #588

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2019
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2019
@@ -105,7 +105,7 @@ type client struct {
}

// resetGroupVersionKind is a helper function to restore and preserve GroupVersionKind on an object.
// TODO(vincepri): Remove this function and its calls once controller-runtime dependencies are upgraded to 1.15.
// TODO(vincepri): Remove this function and its calls once controller-runtime dependencies are upgraded to 1.16?
Copy link
Member

Choose a reason for hiding this comment

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

Can we drop this function?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought we could. I removed it in ffe4ab3 and then reverted that change in the next commit after the tests failed. It seems that 1.15 doesn't fix it.

@vincepri Any clues about where we can look to find the upstream change that would allow us to remove this function?

Copy link
Member

Choose a reason for hiding this comment

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

IIRC this is related to apimachinery version. We're at an offsite for the next couple of days so I won't be able to track this down. Are client-go (https://github.com/kubernetes/client-go/blob/master/INSTALL.md#add-client-go-as-a-dependency) and the related dependencies up to date?

@joelanford
Copy link
Member Author

Now that Kubernetes 1.16 is GA, I'm in agreement with @alvaroaleman to bump directly to that unless there's a good reason not to. On the operator-sdk side, we're looking to bump to 1.16, so anything we can do to assist in that process, let me know!

I'll leave this open for now, but if there's consensus that its okay to go directly to 1.16, we can close this.

@joelanford
Copy link
Member Author

joelanford commented Oct 4, 2019

@DirectXMan12 I rebased this PR to master, updated to kube 1.15.4, and ran it against a POC go-apidiff tool I put together based on golang.org/x/exp/apidiff

Looks like no incompatible changes, only compatible ones:

$ go-apidiff v0.2.2 k8s-1.15.3
<no output>

As a sanity check, I did commit a branch locally with a breaking change to pkg/log/zap to make sure it would catch it:

$ go-apidiff v0.2.2 break-things

sigs.k8s.io/controller-runtime/pkg/log/zap
  Incompatible changes:
  - New: removed

Lastly, to see what changed in the APIs imported by controller-runtime, we can run with --compare-imports:

$ go-apidiff v0.2.2 k8s-1.15.3 --compare-imports

github.com/prometheus/client_golang/prometheus
  Incompatible changes:
  - (*Timer).ObserveDuration: changed from func() to func() time.Duration
  - DefObjectives: removed
  - Handler: removed
  - InstrumentHandler: removed
  - InstrumentHandlerFunc: removed
  - InstrumentHandlerFuncWithOpts: removed
  - InstrumentHandlerWithOpts: removed
  - UninstrumentedHandler: removed

k8s.io/apimachinery/pkg/runtime
  Incompatible changes:
  - Unstructured.NewEmptyInstance: added

k8s.io/apimachinery/pkg/apis/meta/v1
  Incompatible changes:
  - ListInterface.GetRemainingItemCount: added
  - ListInterface.SetRemainingItemCount: added

k8s.io/apimachinery/pkg/api/meta
  Incompatible changes:
  - k8s.io/apimachinery/pkg/api/meta.List.GetRemainingItemCount: added
  - k8s.io/apimachinery/pkg/api/meta.List.SetRemainingItemCount: added

k8s.io/client-go/util/workqueue
  Incompatible changes:
  - Parallelize: removed

k8s.io/apimachinery/pkg/watch
  Incompatible changes:
  - NewStreamWatcher: changed from func(Decoder) *StreamWatcher to func(Decoder, Reporter) *StreamWatcher

@joelanford joelanford changed the title ✨ Bump dependencies for Kubernetes 1.15.3 ✨ Bump dependencies for Kubernetes 1.15.4 Oct 4, 2019
@marun
Copy link
Contributor

marun commented Oct 7, 2019

@joelanford Would you please bump this to 1.16 and perform the same checks for compatibility? There are 2 other PRs proposing to bump to 1.16 but the author of #588 is non-responsive and #618 hasn't been vetted for compatibility.

cc: @DirectXMan12

@enxebre
Copy link
Member

enxebre commented Oct 8, 2019

#618 hasn't been vetted for compatibility.

Just rebased #618 @marun @DirectXMan12 not sure what else is expected there, happy to help to move any of the PRs forward

@marun
Copy link
Contributor

marun commented Oct 8, 2019

#618 hasn't been vetted for compatibility.

Just rebased #618 @marun @DirectXMan12 not sure what else is expected there, happy to help to move any of the PRs forward

See #599 (comment)

@DirectXMan12
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 10, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, joelanford

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

The pull request process is described here

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 10, 2019
@DirectXMan12
Copy link
Contributor

as per our discussion in today's meeting :-)

@k8s-ci-robot k8s-ci-robot merged commit 4ba0a3b into kubernetes-sigs:master Oct 10, 2019
@joelanford joelanford deleted the k8s-1.15.3 branch October 10, 2019 00:55
@enxebre
Copy link
Member

enxebre commented Oct 10, 2019

See #599 (comment)

I run the tool here
#618 (comment)

@DirectXMan12
Copy link
Contributor

shoot. This had a whole bunch of extra stuff in it that shouldn't have gotten in there.

@DirectXMan12
Copy link
Contributor

*a whole bunch of extra commits that should've been squashed.

@DirectXMan12
Copy link
Contributor

In the future, please try to avoid merge commits in your PRs

@joelanford
Copy link
Member Author

Noted. Sorry about that. I'm used to PR commits getting squashed when the PR is merged.

DirectXMan12 pushed a commit that referenced this pull request Jan 31, 2020
Use idiomatic gomega syntax in generated tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants