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

Add config for User-Agent suffix #3650

Merged
merged 3 commits into from
Dec 14, 2023
Merged

Conversation

nojnhuh
Copy link
Member

@nojnhuh nojnhuh commented Dec 13, 2023

Closes #1827

What this PR does / why we need it: This PR adds an AZURE_USER_AGENT_SUFFIX configuration option to allow appending to the default User-Agent used for Azure HTTP clients.

Special notes for your reviewer:

Getting this wired up felt a little clunky, so any feedback on how to smooth that out is welcome.

I also wasn't sure exactly how best to add tests for this config, so any advice there would be appreciated. And tips on exactly where to look with ASO fully up and running to see if the user agent is really being set as expected.

How does this PR make you feel:
gif

If applicable:

  • this PR contains documentation
  • this PR contains tests

@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2023

Codecov Report

Attention: 1075 lines in your changes are missing coverage. Please review.

Comparison is base (f6ab7ee) 53.08% compared to head (99313f5) 53.43%.
Report is 26 commits behind head on main.

Files Patch % Lines
...1/storage/managed_clusters_agent_pool_types_gen.go 70.23% 216 Missing and 140 partials ⚠️
...230202preview/storage/managed_cluster_types_gen.go 58.30% 148 Missing and 108 partials ⚠️
...w/storage/managed_clusters_agent_pool_types_gen.go 53.74% 86 Missing and 50 partials ⚠️
...1/storage/compat/service_mesh_profile_types_gen.go 50.99% 60 Missing and 39 partials ⚠️
...ge/compat/service_mesh_profile_status_types_gen.go 52.97% 56 Missing and 39 partials ⚠️
...ompat/cluster_upgrade_settings_status_types_gen.go 55.55% 24 Missing and 16 partials ⚠️
...orage/compat/cluster_upgrade_settings_types_gen.go 62.22% 20 Missing and 14 partials ⚠️
...rofile_vertical_pod_autoscaler_status_types_gen.go 74.13% 9 Missing and 6 partials ⚠️
...caler_profile_vertical_pod_autoscaler_types_gen.go 74.13% 9 Missing and 6 partials ⚠️
...v1api20210501/storage/managed_cluster_types_gen.go 50.00% 4 Missing and 4 partials ⚠️
... and 4 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3650      +/-   ##
==========================================
+ Coverage   53.08%   53.43%   +0.34%     
==========================================
  Files        1286     1356      +70     
  Lines      406699   460920   +54221     
==========================================
+ Hits       215894   246289   +30395     
- Misses     159804   177613   +17809     
- Partials    31001    37018    +6017     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@theunrepentantgeek theunrepentantgeek left a comment

Choose a reason for hiding this comment

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

Generally looks good, but I have some ideas about an alternative way to plumb the value in.

docs/hugo/content/guide/aso-controller-settings-options.md Outdated Show resolved Hide resolved
@@ -66,7 +66,7 @@ func NewGenericClient(

ua := options.UserAgent
if ua == "" {
ua = userAgent
ua = DefaultUserAgent
Copy link
Member

Choose a reason for hiding this comment

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

This will only be used if the code that calls NewGenericClient() doesn't specify a user agent in the GenericClientOptions struct passed in. I don't think that's ideal, as the configured setting wouldn't be included.

I haven't checked where GenericClientIOptions.UserAgent is set, but I'm wondering if the value used there should be used as a part of the UA, not the whole thing.

v2/internal/reconcilers/arm/arm_client_cache.go Outdated Show resolved Hide resolved
@theunrepentantgeek
Copy link
Member

/ok-to-test sha=fc7fcd4

Copy link
Member

@theunrepentantgeek theunrepentantgeek left a comment

Choose a reason for hiding this comment

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

One minor comment, otherwise looks good. I'll trigger testing and merge once that's addressed.

@theunrepentantgeek
Copy link
Member

/ok-to-test sha=99313f5

@theunrepentantgeek
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@theunrepentantgeek theunrepentantgeek added this pull request to the merge queue Dec 14, 2023
Merged via the queue into Azure:main with commit 86cd2ce Dec 14, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Allow customization of the user agent
3 participants