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

CA: Updating AWS-SDK-Go to 1.26.6 #2750

Conversation

gjtempleton
Copy link
Member

@gjtempleton gjtempleton commented Jan 19, 2020

This brings in the new EC2 DescribeInstanceTypes API.

Also updates the AWS-Manager test for Region fetching due to changes in the SDK in how this is done due to aws/aws-sdk-go#2969

Updating vendor against git@github.com:kubernetes/kubernetes.git:711790af23a231ade04c6c39d6bc5dcf1a996f73 (711790a)

Broken out from the changes to the EC2 instance type list enabled by this SDK update to simplify reviewing of the PRs.

Related to #2696

Also updates the AWS-Manager test for Region fetching due to changes in the SDK in how this is done

Updating vendor against git@github.com:kubernetes/kubernetes.git:711790af23a231ade04c6c39d6bc5dcf1a996f73 (711790a)
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 19, 2020
@gjtempleton gjtempleton changed the title Updating AWS-SDK-Go to 1.26.6 CA: Updating AWS-SDK-Go to 1.26.6 Jan 19, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign losipiuk
You can assign the PR to them by writing /assign @losipiuk in a comment when ready.

The full list of commands accepted by this bot can be found 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

@gjtempleton
Copy link
Member Author

/assign @Jeffwan

@Jeffwan
Copy link
Contributor

Jeffwan commented Jan 20, 2020

Thanks! Seems this is a prerequisite for the next step. Let's merge this dependency update separately. Looks good to me

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 20, 2020
@gjtempleton
Copy link
Member Author

/hold

I've just had a look again and am seeing more changes than I expected, particularly in vendored files under a k8s.io path.

Looks like these reversions are all reversions of changes brought in by #2605 . They seem unrelated to the changes intended by that PR, but could you please confirm @jaypipes just to be certain.

@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 Jan 21, 2020
@jaypipes
Copy link
Contributor

jaypipes commented Jan 28, 2020

/hold

I've just had a look again and am seeing more changes than I expected, particularly in vendored files under a k8s.io path.

Looks like these reversions are all reversions of changes brought in by #2605 . They seem unrelated to the changes intended by that PR, but could you please confirm @jaypipes just to be certain.

@gjtempleton nope, these changes are all good. This PR is adding the new aws-sdk-go 1.26.6 package to the cluster-autoscaler/vendor/github.com/aws/aws-sdk-go directory. This is proper, as the hack/update-vendor.sh script runs go mod vendor after replacing the packages listed in go-mod.extra in the go.mod file generated from upstream k/k. #2605 replaced the "sub-vendored" cluster-autoscaler/cloudprovider/digitalocean/godo package with cluster-autoscaler/vendor/github.com/digitalocean/godo.

In short, we're all good here :)

@gjtempleton
Copy link
Member Author

Brilliant, thanks for the confirmation!

/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 Jan 28, 2020
@k8s-ci-robot
Copy link
Contributor

@gjtempleton: PR needs rebase.

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 the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 10, 2020
@gjtempleton
Copy link
Member Author

Closing as this is made redundant by the merging of #2796

Will port across the code fix for the failing test mentioned in #2797 to a separate PR

@gjtempleton gjtempleton deleted the CA-AWS-DescribeInstanceTypes branch February 18, 2020 09:53
@gjtempleton gjtempleton restored the CA-AWS-DescribeInstanceTypes branch February 18, 2020 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. 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
Development

Successfully merging this pull request may close these issues.

4 participants