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

vendor: Downgrade Azure dependencies #22490

Closed
wants to merge 2 commits into from

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Aug 16, 2019

This is effectively reverting #22247 and #22248


I haven't had chance to debug and fully understand why, but here's what the end-user experience of provider developer looks like (this is for a provider which doesn't depend on the Azure SDK at all - AWS):

go: finding github.com/hashicorp/terraform master
go: downloading github.com/hashicorp/terraform v0.12.7-0.20190816123248-b5508b914224
go: extracting github.com/hashicorp/terraform v0.12.7-0.20190816123248-b5508b914224
build github.com/hashicorp/terraform: cannot load github.com/Azure/go-autorest/autorest: ambiguous import: found github.com/Azure/go-autorest/autorest in multiple modules:
	github.com/Azure/go-autorest v10.15.4+incompatible (/Users/radeksimko/gopath/pkg/mod/github.com/!azure/go-autorest@v10.15.4+incompatible/autorest)
	github.com/Azure/go-autorest/autorest v0.5.0 (/Users/radeksimko/gopath/pkg/mod/github.com/!azure/go-autorest/autorest@v0.5.0)

So the end-user/developer of a provider receives quite a cryptic error message, which is probably solvable by adding replace into their go.mod, but:

  • the solution is not immediately obvious
  • we force majority of our users/developers to do some extra work to comfort the few, which is a pretty high cost to pay for this change
  • we end up with these replace "hacks" sprinkled over all provider codebases, which is far from ideal

I do not know what the right solution is, but I'm proposing to revert these changes until we find a solution which ideally doesn't break most providers' go get UX.

@radeksimko radeksimko added the dependencies Auto-pinning label Aug 16, 2019
@radeksimko radeksimko requested review from vancluever and a team August 16, 2019 13:47
@appilon
Copy link
Contributor

appilon commented Aug 16, 2019

More information on why this is happening can be found here: https://gist.github.com/appilon/1ba757b22845b400aa9841ff54d0f330

To summarize, it seems when upgrading Terraform go modules tries to satisfy both the old version of TF and the new one. This will lead to an ambiguous import of the problematic dependencies, despite this being incorrect, we only need the transitive dependencies of the new Terraform. You can clear up the temporary problem with go mod tidy. This is explained a bit more clearly in the gist.

Copy link
Contributor

@appilon appilon left a comment

Choose a reason for hiding this comment

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

I approve but would love to see how providers who were affected and wanted the change to begin with feel.

@tombuildsstuff
Copy link
Member

@jhendrixMSFT any chance we we get your input on the best path forward here?

To give a little context: Terraform's vendored as a library into it's providers (which can vendor each other, for historical reasons) - so whilst replace might work at the top level, that's going to need to be added to every provider (or ask folks to run go mod tidy to remove the unused dependency) - which isn't ideal

@radeksimko
Copy link
Member Author

cc @tamalsaha

Copy link
Contributor

@vancluever vancluever left a comment

Choose a reason for hiding this comment

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

While this revert is not convenient for me, I recognize that this workflow will be frustrating for the large majority of other providers that may end up running into the same issues that @radeksimko did when trying to update the AWS provider. This number could be quite large, given that there are over 100 official Terraform providers.

Additionally I think we also need to take into account that some providers have tools such as Renovate to automatically update dependencies, and working around this in those workflows might be tougher.

As such, I think in the spirit of a better DX for the majority of our downstream consumers, this is a 👍 for me. From what I understand the provider SDK is also not too far out, which should prevent TF core deps from leaking into providers, effectively making the issue that caused this in the first place moot.

FWIW a downgrade of TF to 0.12.6 in the ACME provider - before these changes were introduced - triggered some Terraform-associated build errors, but the provider built without issue. So the consequences of reversion may not be so bad.

@tamalsaha, Radek has already tagged you but as a note you could probably continue to use the current commit you vendored against a couple of weeks ago without issue, but YMMV and that's not an official recommendation. There is work that should be ready relatively soon to de-couple the provider SDK from the rest of Terraform core, and at that point in time there should be no more dependency conflicts like this. Beyond that, reiterating what was discussed here, we don't make any guarantees of the stability of the packages in Terraform core beyond what is needed to facilitate normal use of providers. Apologies for any inconvenience this may cause!

@tamalsaha
Copy link

tamalsaha commented Aug 16, 2019

It might be worth waiting to hear from @jhendrixMSFT . He is actively working on getting the go.mod issues under control.

@jhendrixMSFT
Copy link

Regarding the ambiguous imports problem, this is being discussed in Azure/go-autorest#414, I will get this deployed in go-autorest today.

@radeksimko radeksimko deleted the vendor-downgrade-azure-deps branch August 20, 2019 14:53
radeksimko added a commit that referenced this pull request Aug 20, 2019
This is to allow Terraform providers to upgrade to at least
one more minor version of the plugin SDK without major UX hiccups.

This concludes (unsuccessful) experiments involving upgrades
to SDK with Azure/go-autorest#455

Even with that patch all providers still experience broken UX
as described in #22490

This downgrade reduces the uncomfort to only a handful of providers
from >100s. The affected providers more or less directly depend on
Azure SDK(s), which is ~8.

Affected providers practically cannot consume Terraform Plugin SDK
with this patch (downgraded Azure SDKs) and can just wait for
extracted Terraform Plugin SDK which is planned to be released soon.

This reverts the following PRs:

 - #22247
 - #22248
 - #22524
 - #22525

and it is otherwise result of the following commands

```
go get github.com/Azure/azure-sdk-for-go@v21.3.0
go get github.com/hashicorp/go-azure-helpers@166dfd221bb2
go mod tidy
```
@ghost
Copy link

ghost commented Sep 20, 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 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dependencies Auto-pinning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants