Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

chore(deps): upgrade go-autorest to v10.15.4 #3843

Merged
merged 1 commit into from
Sep 14, 2018

Conversation

ultimateboy
Copy link
Contributor

What this PR does / why we need it:

Updates the version of go-autorest to the latest release, which includes several bug fixes.

@mboersma
Copy link
Member

/lgtm pending e2e

@tariq1890
Copy link
Contributor

@weinong @jackfrancis @mboersma I see the possibility of some incongruity here as we are updating the go-autorest version and not the Azure Go SDK (which uses the go-autorest library heavily). What are your thoughts about updating the Azure Go SDK as well?

@@ -22,7 +22,7 @@
version = "v19.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's upgrade to v20.1.0

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to do that as well--many services (including containerservices itself) have been updated in the SDK since v19.0.0. I think it would be safest to use the go-autorest version that was actually used to generate the Go SDK, if we can find that out.

Copy link
Member

@mboersma mboersma Sep 14, 2018

Choose a reason for hiding this comment

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

Apparently that was 10.15.3. Close enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked with Martin Strobel earlier, the go sdk maintainer. He says using 10.15.4 of go-autorest along with v20.1.0 of go sdk should be fine.

Copy link
Member

Choose a reason for hiding this comment

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

OTOH, upgrading sdk-for-go has a chance of breaking some API acs-engine is already using (and this has a cascade effect when vendored into AKS). If this is passing, maybe that should be a separate PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure!

@jackfrancis
Copy link
Member

@tariq1890 @mboersma what's the // TODO at this point according to your recommendations?

@tariq1890
Copy link
Contributor

tariq1890 commented Sep 14, 2018

I guess the TODO would be to vendor in an Azure SDK Go release which contains the go-autorest referenced in this PR.

From what I understand, go-autorest 10.15.4 is a bugfix release and that has not been vendored into Azure Go SDK as of yet.

@jackfrancis
Copy link
Member

@tariq1890 can we do that as a follow-up? Or should we do it in this PR?

@tariq1890
Copy link
Contributor

We can do that in another PR. Hopefully, the next release of AzureGo SDK will have the deps set straight.

@jackfrancis
Copy link
Member

/lgtm

@acs-bot acs-bot added the lgtm label Sep 14, 2018
@jackfrancis jackfrancis merged commit f1e44b0 into Azure:master Sep 14, 2018
@acs-bot
Copy link

acs-bot commented Sep 14, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis, mboersma, ultimateboy

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 [jackfrancis,mboersma]

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

@mboersma mboersma mentioned this pull request Sep 20, 2018
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants