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

make user agent string configurable #2816

Closed
wants to merge 2 commits into from

Conversation

jaxxstorm
Copy link

@jaxxstorm jaxxstorm commented Oct 6, 2023

This allows the user agent string to be configurable, meaning providers that "bridge" this provider don't report Terraform as their origin

@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2023

changelog detected ✅

@jacobbednarz
Copy link
Member

jacobbednarz commented Oct 6, 2023

thanks for the PR!

overwriting the user agent completely isn't something we want to support given it is used to assess impact of changes and this could potentially make that harder to find. i'm open to allowing appending to the UA though if there is additional context we can append so we still have the context of the internals we are relying on (we may end up shuffling the order of keys in the UA).

@jacobbednarz
Copy link
Member

also just realised the UA will always send terraform-plugin-sdk in the UA but we could be sending a resource from the framework plugin so we'll need to modify that too.

@jaxxstorm
Copy link
Author

@jacobbednarz this change is primarily so that calls from the pulumi-cloudflare provider are accurately identified. If you have a better mechanism to help identify those rather than via env var, happy to rework this.

@jacobbednarz
Copy link
Member

i'm fine with appending to the UA, just not allowing a complete overwrite. if you want to rework this to allow that, I'm 👍.

@jaxxstorm
Copy link
Author

@jacobbednarz if I rework this so only the first section of the UA is modified, would that work?

E.g.

UserAgentDefault = "%s/%s terraform-plugin-sdk/%s terraform-provider-cloudflare/%s"

Where that first %s is automatically inferred at terraform and it can be overriden as pulumi ?

@jacobbednarz
Copy link
Member

that could work but the use case is very specific. i want to steer clear of pulumi (or any third party for that matter) functionality so that if others have a similar need for their tools, this is something they could lean on.

i'm away from my machine at the moment but was thinking something like this would work

UserAgentDefault = "terraform-provider-cloudflare/%s terraform-plugin-%s/%s"

then allowing appending to the UA via a configuration option (and env var) that defaults to terraform/%s (subbing in the version) if one isn't provided.

if whateverTheVarNameIs {
  finalUA = UserAgentDefault + " " + userProvidedEnVar
} else {
  finalUA = UserAgentDefault + " terraform/%"
}

this then allows anyone that may be using the terraform provider (even via terraform itself) to append a unique and identifiable UA whilst keeping the required information of the underlying context together.

how does that sound? I can PR this at some point tomorrow if I haven't explained it very well.

@jaxxstorm
Copy link
Author

jaxxstorm commented Oct 10, 2023

Yep, I think we're on the same page! If you're open to submitting a PR that implements the logic as you desire, I'm happy to close this. Will leave it open as a reminder for now

@jacobbednarz
Copy link
Member

jacobbednarz commented Oct 11, 2023

i've opened #2831 which should give us what we need here (as well as an upstream bug for the plugin version missing in some cases and not at all present in the framework 🙃). check it out and let me know if that suits your needs!

@cloudflare cloudflare locked as resolved and limited conversation to collaborators Oct 18, 2023
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