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

provider/openstack: Add Terraform version to UserAgent string #14955

Merged
merged 2 commits into from
Jun 2, 2017

Conversation

fatmcgav
Copy link
Contributor

Just because it's useful... :)

@fatmcgav
Copy link
Contributor Author

@jtopjian Thought this would be useful to add :)

@jtopjian
Copy link
Contributor

jtopjian commented Jun 1, 2017

@fatmcgav Yep, definitely useful. Looking at the other implementations of this, they all have HashiCorp in the agent, too, so how about:

HashiCorp-Terraform/" + terraform.VersionString()

@fatmcgav
Copy link
Contributor Author

fatmcgav commented Jun 1, 2017

@jtopjian Yep, can make that tweak... I did look at other providers and didn't see a real standard...

@thomaschaaf
Copy link
Contributor

Wouldn't it make sense to create one helper function to render the user agent for every provider? I don't think it makes sense to manually try to keep the user agent implementation the same?

@jtopjian
Copy link
Contributor

jtopjian commented Jun 1, 2017

Yep, can make that tweak... I did look at other providers and didn't see a real standard...

Yeah, it looks like a mix of agent types. I saw that most of them had "HashiCorp" in the name, which is why I suggested it.

Wouldn't it make sense to create one helper function to render the user agent for every provider? I don't think it makes sense to manually try to keep the user agent implementation the same?

I agree. Or at least standardize on a name.

@stack72 thoughts?

@fatmcgav
Copy link
Contributor Author

fatmcgav commented Jun 1, 2017

There is a 'VersionHeader' const defined here: https://github.com/hashicorp/terraform/blob/master/terraform/version.go#L24

But it still needs to be used to construct a header...
Agree that a helper function would be a useful addition..

Edit: Actually, that const looks like it's specific to the Atlas client... However a similar const for UserAgent would probably be beneficial

@stack72
Copy link
Contributor

stack72 commented Jun 1, 2017

@fatmcgav / @jtopjian IMO, I don't think it's so important - the AWS one is more as a partner thing so they can understand when the tool is used

But it would be nice to follow a similar convention

@fatmcgav
Copy link
Contributor Author

fatmcgav commented Jun 1, 2017

@stack72 / @jtopjian Thoughts on fatmcgav@76fbe83

@stack72
Copy link
Contributor

stack72 commented Jun 1, 2017

That looks good to me :)

@fatmcgav
Copy link
Contributor Author

fatmcgav commented Jun 1, 2017

Cool, ok.. I'll see if I can stick a quick test case on it...

Would it be better as a separate PR, or happy for it to be included in this one?

@stack72
Copy link
Contributor

stack72 commented Jun 1, 2017

You can include it here :)

@fatmcgav
Copy link
Contributor Author

fatmcgav commented Jun 1, 2017

Cool, ok... PR updated :)

@jtopjian
Copy link
Contributor

jtopjian commented Jun 2, 2017

Nice - thanks @fatmcgav! You're on a roll :)

@jtopjian jtopjian merged commit 401c6a9 into hashicorp:master Jun 2, 2017
@ghost
Copy link

ghost commented Apr 11, 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 Apr 11, 2020
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.

4 participants