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.

Sign up for GitHub

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

Jump to bottom

Update dependencies + k8s version #116

Merged
merged 3 commits into from
Mar 5, 2019
Merged

Update dependencies + k8s version #116

merged 3 commits into from
Mar 5, 2019

Conversation

justaugustus
Copy link
Member

@justaugustus justaugustus commented Mar 5, 2019 •
edited
Loading

What this PR does / why we need it:

  • Update the following dependencies:
    • sigs.k8s.io/cluster-api: 0f315e945d5fabe0c805ccf30fa6cf02fac6c0b5 --> c51a9c13aa0ebe6d2d7bef869094b9b78bebcd48
    • github.com/Azure/azure-sdk-for-go: 25.1.0 --> 26.2.0
    • github.com/Azure/go-autorest: 11.4.0 --> 11.5.0
  • Add constraint for k8s.io/client-go at kubernetes-1.13.1
  • Update Kubernetes version from 1.13.3 to 1.13.4

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #93

Release note:

NONE

/assign @tariq1890 @soggiest

@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Mar 5, 2019
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 5, 2019
@k8s-ci-robot k8s-ci-robot added area/provider/azure Issues or PRs related to azure provider size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 5, 2019
vincepri
vincepri reviewed Mar 5, 2019
View reviewed changes
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

One comment, otherwise LGTM

Gopkg.toml Outdated

[[override]]
name = "k8s.io/client-go"
version = "kubernetes-1.13.4"
Copy link
Member

Choose a reason for hiding this comment

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

Are these k8s overrides actually needed? The k8s version should be pulled in from cluster-api which pulls it in from controller-runtime.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should be using overrides.

Copy link
Member Author

Choose a reason for hiding this comment

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

@vincepri / @tariq1890 -- See here in capi:

CAPI is pulling 1.13.1. I'd like to maintain consistency with the k8s version bump in this PR (1.13.4), hence the overrides.

If the capi versions get constraints for those packages, then I can plan to remove these overrides later.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to update CAPI and have that pulled transitively. I don't think we should commit an override dep to master.

Copy link
Contributor

Choose a reason for hiding this comment

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

PR created for this kubernetes-sigs/controller-runtime#349

Let's go with this approach. I've had plenty of dependency nightmares because of overrides.

Copy link
Member

Choose a reason for hiding this comment

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

@justaugustus This is a common problem for any provider, but I agree with @tariq1890 to stick with controller-runtime's version for now. That won't stop you from deploying a cluster with an updated version though.

Copy link
Member Author

Choose a reason for hiding this comment

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

SGTM! I've removed the override, set a constraint for client-go @ kubernetes-1.13.1, and added a TODO to Gopkg.toml which links this comment thread and the dependency bump Tariq PR-ed here: kubernetes-sigs/controller-runtime#349

@tariq1890 / @soggiest -- If everything looks good, this is ready to be re-/lgtmed. :)

soggiest
soggiest approved these changes Mar 5, 2019
View reviewed changes
Copy link
Contributor

@soggiest soggiest left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold
Waiting for a response to Vince's comment

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 5, 2019
@justaugustus
Update dependencies
997e8d1 
Signed-off-by: Stephen Augustus <saugustus@vmware.com>
@justaugustus
clusterctl/examples: Bump kubernetes version to 1.13.4
301ab5d 
Signed-off-by: Stephen Augustus <saugustus@vmware.com>
@justaugustus
dep: Add constraint for client-go (kubernetes-1.13.1), bump dependencies
b2f1a87 
Signed-off-by: Stephen Augustus <saugustus@vmware.com>
@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 5, 2019
tariq1890
tariq1890 approved these changes Mar 5, 2019
View reviewed changes
Copy link
Contributor

@tariq1890 tariq1890 left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justaugustus, tariq1890

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:
  • OWNERS [justaugustus,tariq1890]

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

@justaugustus
Copy link
Member Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 5, 2019
@k8s-ci-robot k8s-ci-robot merged commit bd0edaf into kubernetes-sigs:master Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vincepri vincepri vincepri left review comments

@tariq1890 tariq1890 tariq1890 approved these changes

@soggiest soggiest soggiest approved these changes

@roberthbailey roberthbailey Awaiting requested review from roberthbailey

Assignees

@tariq1890 tariq1890

@soggiest soggiest

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/azure Issues or PRs related to azure provider 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. release-note-none Denotes a PR that doesn't merit a release note. 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.

Add client-go constraint to Gopkg.toml
5 participants
@justaugustus @k8s-ci-robot @tariq1890 @vincepri @soggiest