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

Refactor UserAgent. #219

Merged
merged 1 commit into from
Mar 17, 2021
Merged

Refactor UserAgent. #219

merged 1 commit into from
Mar 17, 2021

Conversation

Integralist
Copy link
Collaborator

Problem: we weren't setting the User-Agent HTTP request header when making requests to the dynamic config endpoint.
Notes: in order to support this change I needed to make a bunch of refactoring. Otherwise without the refactoring we would have had to duplicate the UserAgent logic due to 'cyclic import' issues.

Also, we already had a package named version which is used for a subsequent CLI subcommand. So I had to find an alternative name to encapsulate a bunch of version related types. So I settled on 'revision'. It might not be the best name so I'm open to suggestions.

@Integralist Integralist requested review from phamann, a team, triblondon and kailan and removed request for a team March 17, 2021 11:58
Copy link
Member

@kailan kailan left a comment

Choose a reason for hiding this comment

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

User agent is reported as expected!

Screenshot 2021-03-17 at 12 09 58

Copy link
Member

@kailan kailan left a comment

Choose a reason for hiding this comment

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

Diff looks great! I like that the go-fastly user agent is appended so we can see the full dependency chain.

@Integralist Integralist merged commit 2cc715c into master Mar 17, 2021
@Integralist Integralist deleted the integralist/20210317_useragent branch March 17, 2021 14:59
@Integralist Integralist added the bug Something isn't working label Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants