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

Add provider protocol compatibility UI err msg during registry discovery #19976

Merged
merged 1 commit into from
Jan 11, 2019
Merged

Add provider protocol compatibility UI err msg during registry discovery #19976

merged 1 commit into from
Jan 11, 2019

Conversation

findkim
Copy link
Contributor

@findkim findkim commented Jan 11, 2019

[update] a few followup PRs #19977 #19981 related to this feature

The changes add a UI error during provider discovery with the registry to suggest to users to update constraints with the closest provider version with compatible plugin protocol.

Output of init on TF config with provider null version constraint 1.0.0 and plugin protocol 5

provider "null" {
  version = "1.0.0"
}

screen shot 2019-01-11 at 11 21 49 am

Output of init on TF config with provider null version constraint 3.0.0-pre and plugin protocol 5

provider "null" {
  version = "3.0.0-pre"
}

screen shot 2019-01-11 at 11 21 57 am

Local registry test setup with provider null with versions and varying protocol ranges

{
  "id": "terraform-providers/null",
  "verified": true,
  "versions": [
    ...
    {
      "version": "1.0.0",
      "protocols": ["4"],
      "platforms": [{"os": "darwin", "arch": "amd64"}]
    },
    {
      "version": "2.0.0-alpha",
      "protocols": ["4", "5.0"],
      "platforms": [{"os": "darwin", "arch": "amd64"}]
    },
    {
      "version": "3.0.0-pre",
      "protocols": ["6.0"],
      "platforms": [{"os": "darwin", "arch": "amd64"}
      ]
    }
  ]
}

@apparentlymart apparentlymart requested review from a team and removed request for apparentlymart January 11, 2019 17:48
Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

LGTM!

I left a note inline about filtering prerelease versions but I'm happy to get this merged and then work on that as a separate changeset if you'd like.

We also talked out-of-band about the exact rendering of the error messages; happy also to iterate on that in subsequent commits, since it's not a hard blocker for release.

}
}
return nil, ErrorNoVersionCompatible
if earliest == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing I noticed in your screenshots in the PR is that it proposed an alpha build as the "earliest version" supporting protocol 5. This makes me think we should filter out prereleases from consideration here since the suggestion in the error message (using ~> 2.0) can't actually match prerelease versions... prereleases are only selected if they are specified exactly.

Philosophically too, I think we want to avoid ever promoting the use of prereleases outside of a release notes context where we can give more information about the expectations for the release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great catch & point! I'll merge and iterate on the how pre-releases are handled to never be suggested by Terraform.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow-up PR for posterity #19977

@ghost
Copy link

ghost commented Mar 30, 2020

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 Mar 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants