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 legacy-cloud-provider code to pickup ILB subnet changes. #975

Merged
merged 3 commits into from
Jan 2, 2020
Merged

Update legacy-cloud-provider code to pickup ILB subnet changes. #975

merged 3 commits into from
Jan 2, 2020

Conversation

prameshj
Copy link
Contributor

This change updates the legacy-cloud-providers version that is vendored. This is needed for the l4 ILB implementation in ingress-gce.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 16, 2019
@prameshj
Copy link
Contributor Author

/assign @freehan
@spencerhance

spencerhance
spencerhance reviewed Dec 16, 2019
View reviewed changes
go.mod
k8s.io/cloud-provider v0.0.0
k8s.io/component-base v0.0.0
k8s.io/klog v0.4.0
k8s.io/apimachinery v0.17.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you pin these in the replace block as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Synced up offline. Looks like the versions specified in the replace section are the ones actually used. So we will leave these changes as-is. The only change was to modify "legacy-cloud-providers" version in the replace section. rest of the changes are autogenerated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding to this - I updated klog to v1.0.0 since that is required for logs to print correctly.
I did not pin the other repos- apimachinery, client-go, component-base since our ingress-gce code depends on the currently pinned versions in order to build correctly.

spencerhance
spencerhance approved these changes Dec 17, 2019
View reviewed changes
Copy link
Contributor

@spencerhance spencerhance left a comment

Choose a reason for hiding this comment

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

lgtm

@spencerhance
Copy link
Contributor

@freehan

@prameshj
Copy link
Contributor Author

/hold investigating an issue with logs not printing correctly.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 17, 2019
@prameshj
Set logtostderr to false in logs test
d58211f 
This is true by default in klog 1.0.0
Use the updated source ranges function.
@prameshj
Updated legacy-cloud-providers/gce commit.
7550fb7 
Picked up recent changes in l4 ILB code.
The manual changes were:
k8s.io/legacy-cloud-providers => k8s.io/legacy-cloud-providers v0.0.0-20191114112650-b5fed2ccee23
k8s.io/klog => k8s.io/klog v1.0.0
@prameshj
Autogenerated changes
5554667
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 19, 2019
@prameshj
Copy link
Contributor 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 Dec 19, 2019
@prameshj
Copy link
Contributor Author

@spencerhance @freehan Can you take another look? Thanks!

spencerhance
spencerhance approved these changes Dec 20, 2019
View reviewed changes
Copy link
Contributor

@spencerhance spencerhance left a comment

Choose a reason for hiding this comment

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

seems fine to me

freehan
freehan approved these changes Jan 2, 2020
View reviewed changes
Copy link
Contributor

@freehan freehan 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 k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 2, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: freehan, prameshj

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 Jan 2, 2020
@k8s-ci-robot k8s-ci-robot merged commit c18c674 into kubernetes:master Jan 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spencerhance spencerhance spencerhance approved these changes

@freehan freehan freehan approved these changes

@rramkumar1 rramkumar1 Awaiting requested review from rramkumar1

@thockin thockin Awaiting requested review from thockin

Assignees

@freehan freehan

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. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@prameshj @spencerhance @k8s-ci-robot @freehan