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.

Sign up for GitHub

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

Jump to bottom

Updated the way user agent string gets assigned. #587

Merged
merged 2 commits into from
Nov 30, 2017
Merged

Updated the way user agent string gets assigned. #587

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ac14281
Updated the way user agent string gets assigned.
Nov 29, 2017
1436023
Changed code to make it more readable.
Nov 29, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions azurerm/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,10 +198,14 @@ func withRequestLogging() autorest.SendDecorator {
}

func setUserAgent(client *autorest.Client) {
version := terraform.VersionString()
client.UserAgent = fmt.Sprintf("HashiCorp-Terraform-v%s", version)
// if the user agent already has a value append Terraform user agent string
Copy link
Contributor

Choose a reason for hiding this comment

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

This function seems to be a bit harder to read now, suggestions:

  1. Put assignment statement in the if-clause, will it be harder for debugging coz one line of code contains too much statements ?
  2. No matter tfversion or azureagent, you're appending it into user-agent, can we merge the code to avoid some kind of duplications ? Consider: what if we need to append even more fields here?
  3. "HashiCorp-Terraform-v%s" seems to be a template/format string, shall we put it in constants ?
  4. I can understand the second if-clause that you use assignment then checking coz they're related. But for the first if-clause, the assigment of tfVersion has nothing to do with the later checking, shall we move it to a separated line ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. I did it that way because of comments from my previous PR where it was suggested to put the assignment of the variable in the if block.

  2. I don't really see where it can get duplications, unless this code is called more than once, and if that happens I would consider that a bug. If you wanted to make sure there will not be duplications in the user agent string you should then also do a sub-string check before appending the text to the user-agent.

  3. Yes it is a template string but it is only ever used once in the code, doesn't seem to be a good reason to make this a constant.

  4. Yes, it can be it's own line, but as I stated above I did it this way because of my previous PR comments.

Copy link
Contributor

@metacpp metacpp Nov 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

1/4, if a = xxx; a != "" is very good pattern coz it really saves one line and these 2 lines are contextual. While if b = xxx; a != "", i feel it's very weird. Who gave the suggestion ? tom ? We probably can sync with him...

2, the duplication means the logic to append the string with ; as delimiter. If you're asked to append 10 more strings in the user-agent, you will not like to copy and paste. Anyway, it's just a suggestion.

3, read-only and scope are different concepts, even its scope is function level, you still can use constants.

anyway, not blocking issue, LGTM.

if tfVersion := fmt.Sprintf("HashiCorp-Terraform-v%s", terraform.VersionString()); client.UserAgent != "" {
client.UserAgent = fmt.Sprintf("%s;%s", client.UserAgent, tfVersion)
} else {
client.UserAgent = tfVersion
}

// append the CloudShell version to the user agent
// append the CloudShell version to the user agent if it exists
if azureAgent := os.Getenv("AZURE_HTTP_USER_AGENT"); azureAgent != "" {
client.UserAgent = fmt.Sprintf("%s;%s", client.UserAgent, azureAgent)
}
Expand Down