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

Deprecate anonymous Flag For Provider Configuration #506

Merged
merged 4 commits into from
Jul 14, 2020

Conversation

jcudit
Copy link
Contributor

@jcudit jcudit commented Jul 1, 2020

Looking for wider feedback on this approach. Please raise any concerns with this deprecation. It may be possible to shape v3.0.0 to include resource-level anonymous access if there are use cases that need better representation.

/cc https://github.com/terraform-providers/terraform-provider-github/issues/502#issuecomment-652399602

@jcudit jcudit requested review from radeksimko, paultyng and anGie44 July 1, 2020 13:39
@ghost ghost added the size/XS label Jul 1, 2020
@nwsparks
Copy link

nwsparks commented Jul 1, 2020

Why is this being deprecated? Is there some alternative method of using the provider without providing a token that will be provided?

A common use case for this parameter is to retrieve the github IP range using the github_ip_ranges data source as seen below. If this parameter is removed how is it intended that an operation like this be achieved in the future?

provider "github" {
  anonymous  = true
  individual = true
}

data "github_ip_ranges" "github_ips" {}

@jcudit jcudit changed the base branch from revert-2.9.0 to master July 1, 2020 17:37
@jcudit jcudit force-pushed the deprecate-anonymous branch from da4f6af to 6e9f678 Compare July 1, 2020 17:41
@jcudit
Copy link
Contributor Author

jcudit commented Jul 2, 2020

@nwsparks see this comment for ongoing discussion around this PR.

Copy link
Contributor

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

This looks good syntactically, but I assume that there is an amount of users which use the provider without authentication to obtain the IP ranges, as mentioned in https://github.com/terraform-providers/terraform-provider-github/issues/502

Perhaps the absence of a token can signal anonymous mode . We can use this approach to our benefit to enable token-less operations (like IP Range requests) as a fallback when no token is provided, instead of erroring out.

That sounds like a pragmatic solution. I'm not as closely familiar with the needs of the broader user base or priorities, but it may be worth exploring a solution prior to deprecating this flag.

I'm not sure though how easy it is going to be to implement and if the upstream SDK makes it easy to "lazy load" the authentication only when it's needed. Also there are some benefits to validating credentials early.

Perhaps we could turn this into skip_credentials_validation to reflect how AWS provider deals with similar problem?

@bflad Do you have any wisdom to share around this field and/or how AWS provider users get around this when they need to use https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/ip_ranges ?

It is also worth checking how the Fastly provider deals with this: https://www.terraform.io/docs/providers/fastly/d/ip_ranges.html

and Packet has similar data source: https://www.terraform.io/docs/providers/packet/d/ip_block_ranges.html

@ghost ghost added size/M and removed size/XS labels Jul 4, 2020
@jcudit
Copy link
Contributor Author

jcudit commented Jul 4, 2020

The latest commit explores a solution alongside deprecating the flag. We continue to provide an anonymous mode, but we no longer require explicit configuration. Instead, when the token parameter is empty, anonymous mode is enabled.

Shipping this will help unblock re-instating the v2.9.0 features. Edits here will be preferred when fixing up conflicts when actioning https://github.com/terraform-providers/terraform-provider-github/pull/508.

Copy link
Contributor

@anGie44 anGie44 left a comment

Choose a reason for hiding this comment

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

LGTM - just a comment to add this new provider schema/provider deprecation in the website docs as well

@jcudit jcudit merged commit d9623ba into master Jul 14, 2020
@jcudit jcudit deleted the deprecate-anonymous branch July 14, 2020 17:17
kfcampbell pushed a commit to kfcampbell/terraform-provider-github that referenced this pull request Jul 26, 2022
* Deprecate `anonymous` flag for provider configuration

* Add Configuration Tests

* fixup! Fix Spelling

* fixup! hard-code values for CI environment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants