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

Update github.com/Azure/azure-sdk-for-go@v31.0.0 #22087

Closed
wants to merge 1 commit into from

Conversation

tamalsaha
Copy link

Fix #22085

Signed-off-by: Tamal Saha tamal@appscode.com

@hashicorp-cla
Copy link

hashicorp-cla commented Jul 16, 2019

CLA assistant check
All committers have signed the CLA.

@tamalsaha tamalsaha mentioned this pull request Jul 16, 2019
@tamalsaha
Copy link
Author

Travis tests have passed successfully. I'm not sure why the status is not updating in GitHub.

Fix hashicorp#22085

Signed-off-by: Tamal Saha <tamal@appscode.com>
@jbardin
Copy link
Member

jbardin commented Jul 16, 2019

Hi @tamalsaha,

Thanks for the contribution.

I think this is something we're going to have to look at more closely. The +295,140 lines is a bit disconcerting here (though not necessarily a problem if it's due to some largely unused packages). Also a blanket upgrade of all dependencies between major releases is something we have to carefully review as well, especially since it effects the internal plugin protocol with grpc and probuf updates.

@jbardin jbardin requested a review from a team July 16, 2019 20:49
@tamalsaha
Copy link
Author

Hi @jbardin , FWIW, the changes to unrelated packages are rather changes in patch versions as evident from the diff on the vendor/modules.txt file.

https://github.com/hashicorp/terraform/pull/22087/files?file-filters%5B%5D=.txt&hide-deleted-files=true#diff-31be3fbc3cb3d1a36172f2111034b4ce

When I run go mod tidy, these changes are automatically picked up by go compiler. If you folks prefer, I can use replace rules to keep then same as before.

I understand why dependency changes can be scary. But the longer you wait, the scarier it gets.

@pselle
Copy link
Contributor

pselle commented Jul 29, 2019

I understand why dependency changes can be scary. But the longer you wait, the scarier it gets.

The counterpoint here is that it makes it difficult for us to pin-point the source of an issue when a regression happens and its related to a dependency, if those are updated in one go.

Might I suggest reducing the scope of this changeset, to help us help you, so that we can get this merged, rather than having a large PR that goes stale?

Copy link
Contributor

@pselle pselle left a comment

Choose a reason for hiding this comment

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

Suggesting a reduce in scope of this changeset, or a new PR

@tamalsaha
Copy link
Author

tamalsaha commented Jul 29, 2019

@pselle , All the code change I really want is these 2 lines:
https://github.com/hashicorp/terraform/pull/22087/files#diff-9bedbd138a1872ee7c10f252c8710229R84

tf-update

All the dependency changes are coming from go mod tidy. I have tried to trim those but I was not successful. I am totally cool if you can create a fresh pr with just two-line change. 🙏

@vancluever
Copy link
Contributor

Incidentally, I've just added #22247 as we need it the ACME provider (the outdated deps here are creating conflicts with lego's Azure DNS provider).

Not too sure what else is being done here, but upgrading the deps without changing the API version in the remote state backend seems to work fine (pending results of integration tests). Both the binary and github.com/hashicorp/terraform/backend/remote-state/azure build without issue.

PS: @tamalsaha I'm not too sure why https://github.com/hashicorp/terraform/pull/22087/files#diff-9bedbd138a1872ee7c10f252c8710229R20 was necessary but it kind of creates a circular dependency. Core should not be depending on any providers. Can you tell us why this was needed?

@tamalsaha
Copy link
Author

From #22805, we are working on a project that depends on both terraform and terraform-provider for Azure. We are using Go modules and the latest version of these repos as dependency.

	github.com/hashicorp/terraform v0.12.4
	github.com/terraform-providers/terraform-provider-azurerm v1.31.0

So, I also need to use a common version of github.com/hashicorp/go-azure-helpers. So, I tried to update that to github.com/hashicorp/go-azure-helpers@v0.5.0, @vancluever .

I agree that this repo should not depend on github.com/terraform-providers/terraform-provider-azurerm . Do you think I should create the sender using the following snippet?

import (
"github.com/hashicorp/go-azure-helpers/sender"
)

sender.BuildSender("AzureRM")

If you can also update github.com/hashicorp/go-azure-helpers@v0.5.0 in your pr that will be great.

vancluever added a commit that referenced this pull request Jul 30, 2019
To help address the issues posed on #22087 and #22085.
@vancluever
Copy link
Contributor

@tamalsaha I've added #22248 along with what looks like the necessary corrections to arm_client.go. Should be good to go now.

Note that it's been added off of a separate PR, but it also includes #22247, so if you want to try that with a go get you should be able to.

@tamalsaha
Copy link
Author

Thanks a lot, @vancluever ! Once your prs are merged, this pr can be closed.

vancluever added a commit that referenced this pull request Jul 30, 2019
To help address the issues posed on #22087 and #22085.
vancluever added a commit that referenced this pull request Aug 1, 2019
To help address the issues posed on #22087 and #22085.
@tamalsaha tamalsaha closed this Aug 1, 2019
@ghost
Copy link

ghost commented Sep 1, 2019

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Sep 1, 2019
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.

Build failure
6 participants