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

AWS Cloud Provider Cleanup #14879

Merged
merged 4 commits into from
Oct 12, 2015
Merged

AWS Cloud Provider Cleanup #14879

merged 4 commits into from
Oct 12, 2015

Conversation

tpounds
Copy link
Contributor

@tpounds tpounds commented Sep 30, 2015

This is just some minor cleanup now that the new AWS SDK has been merged. It contains the following two changes:

  1. Removes a field from the AWS services object that is constructor private and only used for testing.
  2. Updates the cloud driver to always use the availability zone and region information obtained from the node's metadata service.

@tpounds tpounds changed the title Aws cloud cleanup [Minor] AWS Cloud Cleanup Sep 30, 2015
@k8s-bot
Copy link

k8s-bot commented Sep 30, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

@tpounds
Copy link
Contributor Author

tpounds commented Sep 30, 2015

cc @justinsb

@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 30, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/L

@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 2, 2015
@tpounds tpounds changed the title [Minor] AWS Cloud Cleanup AWS Cloud Provider Cleanup Oct 7, 2015
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 7, 2015
@tpounds
Copy link
Contributor Author

tpounds commented Oct 8, 2015

@justinsb: Any feedback on these changes?

@justinsb
Copy link
Member

justinsb commented Oct 9, 2015

Hi - @tpounds sorry for the delay and thanks for these fixes! These look good, though I think 9c59dc707d32f38b2a89c5274424466cb84508ac would benefit from being its own PR, as I'm less certain about it. The others are all self-evident improvements, so without 9c59dc707 it would be an easy LGTM.

@tpounds
Copy link
Contributor Author

tpounds commented Oct 9, 2015

@justinsb: Thanks for the feedback! I'll cherry-pick 9c59dc7 into a separate pull request and we can discuss the merits or lack there of elsewhere. :)

tpounds added 4 commits October 9, 2015 10:19
@tpounds
Remove private struct field. It is only used at construction.
326dd7c
@tpounds
Remove unnecessary describe VPC call.
f71533c 
The external DescribeVPCs call is unnecessary since only the VPC ID
is used and it is retrieved from the EC2 metadata service.
@tpounds
Remove unused method.
4407ca9
@tpounds
Remove unreachable panic statement.
5d5013a 
The availability zone always exist since it is retrieved
from the instance's EC2 metadata service during cloud
provider construction.
@tpounds tpounds mentioned this pull request Oct 9, 2015
Closed
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 9, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/M

@justinsb
Copy link
Member

justinsb commented Oct 9, 2015

Thanks for making this one easier by separating that one commit! LGTM.

@justinsb justinsb added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 9, 2015
@k8s-github-robot
Copy link

@k8s-bot ok to test

pr builder appears to be missing, activating due to 'lgtm' label.

@k8s-bot
Copy link

k8s-bot commented Oct 9, 2015

GCE e2e test build/test passed for commit 5d5013a.

@k8s-github-robot
Copy link

The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label?

dchen1107 added a commit that referenced this pull request Oct 12, 2015
@dchen1107
Merge pull request #14879 from tpounds/aws-cloud-cleanup
80bf32c 
AWS Cloud Provider Cleanup
@dchen1107 dchen1107 merged commit 80bf32c into kubernetes:master Oct 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@justinsb justinsb

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@tpounds @k8s-bot @k8s-github-robot @justinsb @k8s-teamcity-mesosphere @googlebot @dchen1107