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

Why set a UA if one is already supplied #77

Merged
merged 1 commit into from
Jun 17, 2017

Conversation

Robbilie
Copy link
Contributor

some monitoring software has to prioritize User-Agent, X-User-Agent or possibly some query string parameter where youd typically rank qs and X-User-Agent a higher prio than the normal UA because in some environments (browsers) you cant set the UA.

In such a (not uncommon) setup "resty" would show up as the UA which is not helpful at all. The user of resty already supplies a UA, there is no need to add another header with the resty UA.

some monitoring software has to prioritize User-Agent, X-User-Agent or possibly some query string parameter where youd typically rank qs and X-User-Agent a higher prio than the normal UA because in some environments (browsers) you cant set the UA.

In such a (not uncommon) setup "resty" would show up as the UA which is not helpful at all. The user of resty already supplies a UA, there is no need to add another header with the resty UA.
Copy link
Member

@jeevatkm jeevatkm left a comment

Choose a reason for hiding this comment

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

Thank for the PR.

@@ -79,8 +79,6 @@ func parseRequestHeader(c *Client, r *Request) error {

if IsStringEmpty(hdr.Get(hdrUserAgentKey)) {
hdr.Set(hdrUserAgentKey, fmt.Sprintf(hdrUserAgentValue, Version))
} else {
hdr.Set("X-"+hdrUserAgentKey, fmt.Sprintf(hdrUserAgentValue, Version))
Copy link
Member

Choose a reason for hiding this comment

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

@Robbilie Thank you for the PR. I have read your explanation and understood.

Instead removing, please change to following:

else {
    hdr.Add(hdrUserAgentKey, fmt.Sprintf(hdrUserAgentValue, Version))
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what does that do? Append to the UA? If so I would also advice against this since it leaks information about the client/version (just like you shouldnt expose your webservers or os version on publicly visible status pages).

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it appends to it. For server I agree, we shouldn't expose it. But server should know about the client details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dont want to have that part included in my UA and i would leave that option to the user to include it. If you want to give the user the option, feel free to add a config flag to enable the appending to the UA, I wont and I cant since i am not a go programmer.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. I will merge this PR. Thank you for your time. Yes, if user want they can add it.

@jeevatkm jeevatkm merged commit 2876b89 into go-resty:master Jun 17, 2017
@jeevatkm jeevatkm modified the milestone: v0.13 Milestone Jun 21, 2017
@renathoaz
Copy link

renathoaz commented Dec 21, 2017

@jeevatkm Could you set an option to disable the default UA header? I would like not to set the UA and I don't wanna have the:

User-Agent: go-resty v1.0 - https://github.com/go-resty/resty

is it possible or already implemented?

@jeevatkm
Copy link
Member

@RenathoAzevedo you can set your own user agent header. If you provide one resty will not set default UA.

@renathoaz
Copy link

@jeevatkm yes, I'm aware of it, but I would like not to have one set and neither default.

@jeevatkm
Copy link
Member

@RenathoAzevedo Okay, currently no settings to exclude default UA header however please make use of SetPreRequestHook to remove UA header.

Will add an optional boolean flag to disable default UA. Can you create an issue for this this option?

@Robbilie
Copy link
Contributor Author

@RenathoAzevedo not setting a UA is bad practice, just saying…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants