Skip to content

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

maastha
Copy link
Collaborator

@maastha maastha commented Apr 11, 2025

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:

User-Agent: terraform-provider-mongodbatlas/devel Terraform/1.9.3

Updated format if PreviewProviderV2AdvancedCluster is enabled:

User-Agent: terraform-provider-mongodbatlas/devel Terraform/1.9.3 IsAdvancedClusterPreview/true

Link to any related issue(s): CLOUDP-309433

Type of change:

  • Bug fix (non-breaking change which fixes an issue). Please, add the "bug" label to the PR.
  • New feature (non-breaking change which adds functionality). Please, add the "enhancement" label to the PR. A migration guide must be created or updated if the new feature will go in a major version.
  • Breaking change (fix or feature that would cause existing functionality to not work as expected). Please, add the "breaking change" label to the PR. A migration guide must be created or updated.
  • This change requires a documentation update
  • Documentation fix/enhancement

Required Checklist:

  • I have signed the MongoDB CLA
  • I have read the contributing guides
  • I have checked that this change does not generate any credentials and that they are NOT accidentally logged anywhere.
  • I have added tests that prove my fix is effective or that my feature works per HashiCorp requirements
  • I have added any necessary documentation (if appropriate)
  • I have run make fmt and formatted my code
  • If changes include deprecations or removals I have added appropriate changelog entries.
  • If changes include removal or addition of 3rd party GitHub actions, I updated our internal document. Reach out to the APIx Integration slack channel to get access to the internal document.

Further comments

parts = append(parts, part)
userAgent := strings.Join(versions, " ")
if len(metadata) > 0 {
userAgent = fmt.Sprintf("%s (%s)", userAgent, strings.Join(metadata, "; "))
Copy link
Member

@lantoli lantoli Apr 14, 2025

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)

Copy link
Collaborator Author

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

@maastha maastha marked this pull request as ready for review April 14, 2025 18:26
@maastha maastha requested a review from a team as a code owner April 14, 2025 18:26
@marcosuma
Copy link
Collaborator

Can we double check this is not going to break any existing script that will eg start consider Terraform/1.9.3 IsAdvancedClusterPreview/true as a whole new entry? chances are low but good to ask Victor I guess

@@ -6,10 +6,10 @@ import (
"fmt"
"net/http"
"net/url"
"strconv"
Copy link
Member

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?

Copy link
Collaborator Author

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

@maastha
Copy link
Collaborator Author

maastha commented Apr 16, 2025

Can we double check this is not going to break any existing script that will eg start consider Terraform/1.9.3 IsAdvancedClusterPreview/true as a whole new entry? chances are low but good to ask Victor I guess

@marcosuma checked with Victor, added link to the slack thread in the ticket

@maastha
Copy link
Collaborator Author

maastha commented Apr 16, 2025

As discussed, holding off on merging this until after 1.33 release

Copy link
Contributor

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!

@github-actions github-actions bot added the stale label Apr 22, 2025
@EspenAlbert EspenAlbert added the not_stale Not stale issue or PR label Apr 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not_stale Not stale issue or PR stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants