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

Use the latest versions of azure go sdk and go-autorest #5015

Merged
merged 14 commits into from Jan 28, 2019

Conversation

tariq1890
Copy link
Contributor

@tariq1890 tariq1890 commented Dec 19, 2018

Update Azure-Go-SDK to 23.2.0
Update Go-Autorest to 11.2.8

The current build of Azure Go SDK is over 2 years old. It would be in the best interests to keep the Azure deps updated. Also, the querying of PowerStates requires us to use more recent. versions of the Go SDK (refer to #4908 )

Feedback is appreciated :). The changes worth noting can be seen in azure.go and azure_test.go. There are changes in the method signatures and type structure which I thought might be of interest to the reviewers.

@tariq1890 tariq1890 changed the title Use latest version azure go sdk and go-autorest [WIP]Use latest version azure go sdk and go-autorest Dec 19, 2018
discovery/azure/azure.go Show resolved Hide resolved
discovery/azure/azure.go Outdated Show resolved Hide resolved
Signed-off-by: tariqibrahim <tariq.ibrahim@microsoft.com>
Copy link
Member

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

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

This change bumps a few dependencies like github.com/grpc-ecosystem/grpc-gateway and google.golang.org/grpc because of the newly added dependency on go.opencensus.io (through github.com/Azure/go-autorest -> contrib.go.opencensus.io/exporter/ocagent).

And because github.com/Azure/go-autorest doesn't use Go modules (unfortunately), Go can't determine which version of contrib.go.opencensus.io/exporter/ocagent it should depend on and it takes the latest tag. To match with the Gopkg.lock, we should pin contrib.go.opencensus.io/exporter/ocagent to v0.2.0.

discovery/azure/azure.go Outdated Show resolved Hide resolved
@simonpasquier
Copy link
Member

Filed Azure/go-autorest#353

@simonpasquier
Copy link
Member

cc @sylr @marratj @jannickfahlbusch @johscheuer who have recently contributed fixes and improvements to the Azure SD and might be able to test this PR once it is ready.

tariqibrahim added 2 commits December 20, 2018 11:49
Signed-off-by: tariqibrahim <tariq.ibrahim@microsoft.com>
Signed-off-by: tariqibrahim <tariq.ibrahim@microsoft.com>
@jannickfahlbusch
Copy link
Contributor

jannickfahlbusch commented Dec 20, 2018

I can test this after this got merged 😊

However, as #4719 just fixes a silenced error everything should be fine from this PR.

@tariq1890
Copy link
Contributor Author

Just to make this clear, this PR is a work in progress. So it may go through significant changes. Reviews are always welcomed from everyone :)

discovery/azure/azure.go Outdated Show resolved Hide resolved
tariqibrahim added 2 commits December 21, 2018 23:31
Signed-off-by: tariqibrahim <tariq.ibrahim@microsoft.com>
@simonpasquier
Copy link
Member

@tariq1890 could you resolve the conflicts?

@tariq1890
Copy link
Contributor Author

@simonpasquier Thanks for the follow-up on this PR. I will get to working on this :)

Signed-off-by: tariqibrahim <tariq181290@gmail.com>
@tariq1890 tariq1890 changed the title [WIP]Use latest version azure go sdk and go-autorest Use latest version azure go sdk and go-autorest Jan 17, 2019
@tariq1890
Copy link
Contributor Author

Ready for another round of review @simonpasquier @brian-brazil

@tariq1890 tariq1890 changed the title Use latest version azure go sdk and go-autorest Use the latest versions of azure go sdk and go-autorest Jan 17, 2019
discovery/azure/azure.go Outdated Show resolved Hide resolved
discovery/azure/azure.go Outdated Show resolved Hide resolved
@tariq1890 tariq1890 force-pushed the latest_azure branch 2 times, most recently from 42e22de to a1c6139 Compare January 17, 2019 17:56
Signed-off-by: tariqibrahim <tariq181290@gmail.com>
discovery/azure/azure.go Outdated Show resolved Hide resolved
discovery/azure/azure.go Outdated Show resolved Hide resolved
Signed-off-by: tariqibrahim <tariq181290@gmail.com>
Signed-off-by: tariqibrahim <tariq181290@gmail.com>
Copy link
Member

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

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

This LGTM. As I wrote in #4719, we're in master freeze so any merging is delayed.

Signed-off-by: tariqibrahim <tariq181290@gmail.com>
@tariq1890
Copy link
Contributor Author

tariq1890 commented Jan 21, 2019

@simonpasquier I addressed the additional review comments as suggested by you in #4719

@jannickfahlbusch I request you to be ready to rebase your PR as soon as this is merged, so that your fix may be moved in at the earliest.

Also requesting @sylr @jannickfahlbusch @marratj to validate this. I have tested this on my local setup and it seems to work fine.

@marratj
Copy link
Contributor

marratj commented Jan 22, 2019

@tariq1890 I just tested your latest commit in our Azure environment and discovery works fine :-)

@tariq1890
Copy link
Contributor Author

@brian-brazil Requesting your review

@brian-brazil brian-brazil merged commit f4275d2 into prometheus:master Jan 28, 2019
@brian-brazil
Copy link
Contributor

Thanks!

@tariq1890 tariq1890 deleted the latest_azure branch January 28, 2019 18:42
sylr pushed a commit to sylr/prometheus that referenced this pull request Feb 24, 2019
)

Signed-off-by: tariqibrahim <tariq181290@gmail.com>
(cherry picked from commit f4275d2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants