-
Notifications
You must be signed in to change notification settings - Fork 190
chore: Updates user agent to include if PreviewProviderV2AdvancedCluster env var is enabled #3275
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
base: master
Are you sure you want to change the base?
Conversation
internal/config/client.go
Outdated
parts = append(parts, part) | ||
userAgent := strings.Join(versions, " ") | ||
if len(metadata) > 0 { | ||
userAgent = fmt.Sprintf("%s (%s)", userAgent, strings.Join(metadata, "; ")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not follow the current format? e.g.
User-Agent: terraform-provider-mongodbatlas/devel Terraform/1.9.3 IsPreviewV2AdvancedClusterEnabled/true
(also consider reducing IsPreviewV2AdvancedClusterEnabled to something shorter but still easy to know what it is)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keeping existing format & renamed to IsAdvancedClusterPreview
Also will also include value if false as:
User-Agent: terraform-provider-mongodbatlas/devel Terraform/1.9.3 IsAdvancedClusterPreview/false
Can we double check this is not going to break any existing script that will eg start consider |
@@ -6,10 +6,10 @@ import ( | |||
"fmt" | |||
"net/http" | |||
"net/url" | |||
"strconv" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible to have some unit and/or acc tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really, since the user agent method uses some static variables & TF version, it's tricky to add unit tests for this
@marcosuma checked with Victor, added link to the slack thread in the ticket |
As discussed, holding off on merging this until after 1.33 release |
This PR has gone 7 days without any activity and meets the project’s definition of "stale". This will be auto-closed if there is no new activity over the next 7 days. If the issue is still relevant and active, you can simply comment with a "bump" to keep it open, or add the label "not_stale". Thanks for keeping our repository healthy! |
Description
Updates user agent to include if PreviewProviderV2AdvancedCluster env var is enabled.
NOTE: This change will include this flag in ALL API calls from ALL resources & not limited to only advanced_cluster_tpf. To get actual analytics around this variable, we may need to filter by resource & org IDs.
Current user agent:
Updated format if PreviewProviderV2AdvancedCluster is enabled:
Link to any related issue(s): CLOUDP-309433
Type of change:
Required Checklist:
Further comments