Set custom User Agent for Terraform providers #729
Conversation
|
Warning Rate limit exceeded@aknysh has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 51 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily focus on enhancing the handling of user agent strings within the Atmos CLI, specifically for Terraform commands. Modifications include the introduction of new fields and flags for user agent management, updates to configuration files, and documentation enhancements to clarify these new features. The changes also involve a shift in how version information is managed across various files, reflecting a restructuring of the codebase. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Cerebrovinny
force-pushed
the
feat/dev-2337
branch
from
c772c71
to
d351e23
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range comments (1)
internal/exec/terraform.go (1)
Line range hint
258-271: Integration looks good. Consider adding a debug log for the new environment variable.The placement of the new code block for setting
TF_APPEND_USER_AGENTis appropriate and integrates well with the existing function flow. The debug logging that follows will include the new environment variable if set.To improve debugging, consider adding a specific debug log for the new
TF_APPEND_USER_AGENTsetting:if strings.TrimSpace(cliConfig.Components.Terraform.AppendUserAgent) != "" { + u.LogDebug(cliConfig, fmt.Sprintf("Setting custom user agent: %s", strings.TrimSpace(cliConfig.Components.Terraform.AppendUserAgent))) info.ComponentEnvList = append( info.ComponentEnvList, fmt.Sprintf("TF_APPEND_USER_AGENT=%s", strings.TrimSpace(cliConfig.Components.Terraform.AppendUserAgent)), ) }This addition will make it easier to confirm when the custom user agent is being set.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- internal/exec/terraform.go (1 hunks)
- pkg/schema/schema.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
pkg/schema/schema.go (1)
65-65: LGTM: New field added to support custom user agent.The addition of the
AppendUserAgentfield to theTerraformstruct aligns with the PR objective of setting a custom user agent for the Terraform AWS provider. This change allows for the configuration of a custom user agent string, which can be used to identify and track Atmos-initiated operations.To ensure this change is properly utilized, let's verify if it's being used in the relevant parts of the codebase:
✅ Verification successful
Verified:
AppendUserAgentis properly utilized in the codebase.
internal/exec/terraform.gohandles the custom user agent logic using theAppendUserAgentfield.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of the new AppendUserAgent field rg --type go "AppendUserAgent" -C 3Length of output: 1623
|
I guess we should move the static version to dynamic using |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
- internal/exec/terraform.go (1 hunks)
- pkg/config/config.go (1 hunks)
- pkg/config/utils.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
internal/exec/terraform.go (1)
258-266: LGTM! The environment variableTF_APPEND_USER_AGENTis correctly set with proper precedenceThe implementation properly sets the
TF_APPEND_USER_AGENTenvironment variable, respecting the precedence of the environment variable over the configuration file as intended. This aligns well with the PR objective to distinguish Atmos-initiated actions.
osterman
changed the title
|
@Cerebrovinny thanks for the screenshot. Looks like we're not following the general UA convention. Let's change to: For reference, see: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/User-Agent |
Cerebrovinny
force-pushed
the
feat/dev-2337
branch
from
0a84332
to
eb36074
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (12)
- .goreleaser.yml (1 hunks)
- Makefile (1 hunks)
- build.sh (1 hunks)
- cmd/version.go (2 hunks)
- internal/exec/terraform.go (1 hunks)
- internal/exec/utils.go (2 hunks)
- pkg/config/config.go (4 hunks)
- pkg/config/const.go (1 hunks)
- pkg/config/utils.go (1 hunks)
- pkg/schema/schema.go (2 hunks)
- pkg/version/version.go (1 hunks)
- website/docs/quick-start/install-atmos.mdx (1 hunks)
🔇 Additional comments (14)
pkg/version/version.go (1)
3-5: 🛠️ Refactor suggestionEnhance version documentation and consider semantic versioning for default.
The current implementation has a few potential improvements:
- The documentation should mention that this version is used in the Terraform user agent
- The default version "0.0.1" might not be the best choice for a fallback version
Consider these improvements:
-// Version holds the current version of the Atmos CLI. -// It can be set dynamically during build time using ldflags. -var Version = "0.0.1" // Default version; will be overridden during build +// Version holds the current version of the Atmos CLI. +// It can be set dynamically during build time using ldflags. +// This version is used in various places including the Terraform user agent string +// to identify requests made by Atmos. +// +// Example build command: +// go build -ldflags "-X github.com/cloudposse/atmos/pkg/version.Version=1.2.3" +var Version = "dev" // Default version for development; will be overridden during buildUsing "dev" as the default version is a common practice that:
- Clearly indicates when the version wasn't properly set during build
- Follows the convention used by many Go projects
- Makes it obvious in logs and user agents when a development build is being used
Let's verify if the version is being properly set during build:
✅ Verification successful
Let me verify if the version is used in the Terraform user agent by searching for its usage.
Enhance version documentation and update default version
The version is indeed used in the Terraform user agent string (
pkg/config/config.go) and is properly set during builds via ldflags in multiple places (Makefile, build.sh, and .goreleaser.yml). The suggested improvements are valid and beneficial:-// Version holds the current version of the Atmos CLI. -// It can be set dynamically during build time using ldflags. -var Version = "0.0.1" // Default version; will be overridden during build +// Version holds the current version of the Atmos CLI. +// It can be set dynamically during build time using ldflags. +// This version is used in various places including the Terraform user agent string +// to identify requests made by Atmos. +// +// Example build command: +// go build -ldflags "-X github.com/cloudposse/atmos/pkg/version.Version=1.2.3" +var Version = "dev" // Default version for development; will be overridden during build🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if version is being set in build configuration # Test 1: Check Makefile for version setting echo "Checking Makefile for version setting:" rg -A 2 "ldflags.*version.Version" # Test 2: Check .goreleaser.yml for version setting echo "Checking .goreleaser.yml for version configuration:" rg -A 2 "ldflags" .goreleaser.ymlLength of output: 1145
Script:
#!/bin/bash # Search for Version usage in terraform-related code rg -B 2 -A 2 "Version" --type go | grep -v "^version.go" # Search specifically for user agent configuration rg -B 2 -A 2 "UserAgent|User-Agent" --type goLength of output: 14350
build.sh (1)
5-5: LGTM! Good package organization.The change to move version information from
cmdto a dedicatedversionpackage follows good separation of concerns and aligns with Go best practices. This restructuring will make version management more maintainable, especially for the new Terraform user agent feature.Let's verify the version package structure:
✅ Verification successful
The version package restructuring is correctly implemented
The verification confirms that:
- The version information has been properly moved to a dedicated package at
pkg/version/version.go- All necessary imports and references have been updated across the codebase
- The build flags in
build.sh,Makefile, and.goreleaser.ymlcorrectly reference the new version package path- The version variable is properly used in user agent strings and CLI version display
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the version package structure and usage # Test 1: Check if version package exists echo "Checking version package structure..." fd --type f "^.*\.go$" version/ # Test 2: Check version usage across codebase echo "Checking version usage..." rg "version\.Version" --type goLength of output: 942
Script:
#!/bin/bash # Let's check the actual structure and content of version-related files # Check for any Go files containing "Version" declaration echo "Checking Version variable declarations..." ast-grep --pattern 'var Version string' # Check imports of the version package echo "Checking version package imports..." rg '^import.*".*version"' --type go # Check if version might be in a different directory echo "Searching for version-related Go files..." fd -e go -x grep -l "Version.*string" {}Length of output: 508
Script:
#!/bin/bash # Let's examine the actual version-related code structure # Check the content of files that might contain version information echo "Checking content of potential version files..." rg "var Version" --type go -C 3 # Check the git history to see if version package was recently moved or added echo "Checking recent version-related changes..." git log -p --grep="version" -n 1 # Check if version might be defined in a different way echo "Checking other version declarations..." ast-grep --pattern 'Version = $_'Length of output: 6269
Makefile (1)
21-21: LGTM! Good architectural improvement.Moving the version information to a dedicated
pkg/versionpackage improves the separation of concerns and makes it more accessible for features like the custom user agent.Let's verify the version package exists:
✅ Verification successful
Let me check the content of the version package to verify the Version variable declaration, as the previous ast-grep pattern might have been too strict.
✅ Version package and variable verified successfully
The
pkg/versionpackage exists with the correctVersionvariable that will be populated during build time via ldflags, confirming this is a valid architectural improvement.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the version package and its Version variable # Test 1: Check if the version package exists fd --type f "^.*\.go$" "pkg/version" # Test 2: Verify the Version variable declaration ast-grep --pattern 'var Version string'Length of output: 101
Script:
#!/bin/bash # Check the content of version.go and look for Version variable with more flexible patterns cat pkg/version/version.go # Alternative pattern matching for Version variable ast-grep --pattern 'var Version = $_' ast-grep --pattern 'var Version $_'Length of output: 413
cmd/version.go (2)
13-13: LGTM! Clean version package import.The addition of the version package import follows Go conventions and supports centralized version management.
29-29: LGTM! Version usage properly refactored.The changes effectively utilize the centralized version from the version package while maintaining all existing functionality. This refactoring supports the broader goal of implementing consistent version reporting across the application, including the new Terraform user agent feature.
Also applies to: 36-36
.goreleaser.yml (1)
26-26: LGTM! Verify version import path consistency.The move to a dedicated version package is a good practice for better code organization.
Let's verify that all version references have been updated consistently:
✅ Verification successful
Let me gather more information to ensure all version-related code has been properly migrated.
Let me check the content of these version-related files to ensure proper migration.
Version package migration is complete and consistent
The verification confirms that:
- The version declaration has been properly moved to
pkg/version/version.go- The old
cmd.Versionis no longer used anywhere in the codebase- The new
pkg/version.Versionis consistently referenced in build files (Makefile, goreleaser.yml)- The
cmd/version.gocommand properly imports and uses the new version package🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to the old version import path # and confirm the new version package is properly referenced. # Test 1: Check for any remaining references to the old version path echo "Checking for old version path references..." rg "github.com/cloudposse/atmos/cmd\.Version" # Test 2: Verify the new version path is used consistently echo "Verifying new version path usage..." rg "github.com/cloudposse/atmos/pkg/version\.Version"Length of output: 604
Script:
#!/bin/bash # Let's check for any version-related code in both old and new locations # and verify the complete migration # Check for version-related files in cmd directory echo "Checking version files in cmd directory..." fd "version" "cmd" # Check for version-related files in pkg directory echo "Checking version files in pkg directory..." fd "version" "pkg" # Look for any version-related imports or references echo "Checking version-related imports..." rg -l "\"version\"" --type go # Check for any Version variable declarations echo "Checking Version variable declarations..." ast-grep --pattern 'var Version string'Length of output: 556
Script:
#!/bin/bash # Check the content of version-related files echo "Content of cmd/version.go:" cat cmd/version.go echo -e "\nContent of pkg/version/version.go:" cat pkg/version/version.go echo -e "\nChecking for version imports in cmd/version.go:" rg "import.*version" cmd/version.go -A 5Length of output: 1811
pkg/config/const.go (1)
30-30: LGTM! Verify constant usage.The new constant follows the established naming and value patterns for CLI flags.
Let's verify how this constant is being used:
✅ Verification successful
Constant is properly integrated and used
The
AppendUserAgentFlagconstant is correctly integrated into the codebase:
- Added to the list of common flags
- Properly handled in argument parsing with both formats (
--append-user-agent valueand--append-user-agent=value)- Value is stored in
info.AppendUserAgentfor further processing🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of AppendUserAgentFlag constant # Expected: Should be used in command-line processing and Terraform execution # Test 1: Find direct references to the constant rg -l "AppendUserAgentFlag" # Test 2: Find any hardcoded instances of the flag that should use the constant instead rg -l -- "--append-user-agent"Length of output: 121
Script:
#!/bin/bash # Let's check how the constant is used in internal/exec/utils.go rg -A 5 -B 5 "AppendUserAgentFlag" internal/exec/utils.goLength of output: 906
pkg/config/config.go (2)
19-19: LGTM! Good use of version package.The addition of the version package import enables dynamic version number usage, which is the right approach.
47-47: LGTM! Follows recommended user agent format.The user agent format follows the recommended pattern and correctly uses dynamic versioning.
internal/exec/terraform.go (1)
258-266: 🧹 Nitpick (assertive)LGTM! Consider adding string trimming and format validation.
The implementation correctly handles the precedence chain for the user agent (ENV var > atmos.yaml > default). However, consider these improvements:
- Add string trimming to handle potential whitespace:
- appendUserAgent := cliConfig.Components.Terraform.AppendUserAgent + appendUserAgent := strings.TrimSpace(cliConfig.Components.Terraform.AppendUserAgent) - if envUA, exists := os.LookupEnv("TF_APPEND_USER_AGENT"); exists && envUA != "" { + if envUA, exists := os.LookupEnv("TF_APPEND_USER_AGENT"); exists && strings.TrimSpace(envUA) != "" { - appendUserAgent = envUA + appendUserAgent = strings.TrimSpace(envUA)
- Add validation for the user agent format:
if appendUserAgent != "" { + // Validate format: "Atmos/1.181 (Cloud Posse; +https://atmos.tools)" + if !strings.HasPrefix(appendUserAgent, "Atmos/") { + return fmt.Errorf("invalid user agent format: %s. Expected format: Atmos/VERSION (Cloud Posse; +https://atmos.tools)", appendUserAgent) + } info.ComponentEnvList = append(info.ComponentEnvList, fmt.Sprintf("TF_APPEND_USER_AGENT=%s", appendUserAgent)) }Let's verify the user agent format in the codebase:
pkg/config/utils.go (1)
330-334: LGTM! Implementation aligns with PR objectives.The environment variable handling follows the established pattern in the codebase and correctly implements the custom user agent functionality for Terraform providers.
pkg/schema/schema.go (2)
65-65: LGTM! Field addition for user agent configuration.The
AppendUserAgentfield in theTerraformstruct is well-placed and properly tagged for YAML/JSON/mapstructure serialization, allowing users to configure the Terraform user agent through configuration files.
136-136: LGTM! Field addition for CLI-based user agent override.The
AppendUserAgentfield in theArgsAndFlagsInfostruct enables CLI-based override of the Terraform user agent, providing flexibility in how the user agent is specified.internal/exec/utils.go (1)
38-38: LGTM: AppendUserAgentFlag added to commonFlags.The flag is correctly added to the list of common flags that Atmos processes internally.
There was a problem hiding this comment.
@Cerebrovinny thanks, looks good.
Please update the docs:
- https://atmos.tools/cli/commands/terraform/usage - describe the new command line flag
--append-user-agent
Please update the help:
- https://github.com/cloudposse/atmos/blob/main/internal/exec/help.go#L48 - describe the new command line flag
--append-user-agent
aknysh
added
no-release
aknysh
changed the title
aknysh
added
minor
|
These changes were released in v1.96.0. |
What
TF_APPEND_USER_AGENTenvironment variable, which Terraform uses when interacting with the AWS provider.TF_APPEND_USER_AGENT, allowing further flexibility in monitoring or tracking specific operations.atmos.yamlas it is already set by default within the code.atmos {currentVersion}as the default.version, has been created and migrated out of thecmdpackage. This change was necessary to avoid dependency cycles that would have blocked usage in other components.versionpackage were identified in the following repository: terraform-provider-utils.Why
Proof of working code
New tests created after the requested changes
references
#DEV-2337
Summary by CodeRabbit
Release Notes
New Features
--append-user-agentflag for customizing User-Agent strings in Terraform requests.append_user_agentconfiguration option inatmos.yamlfor user agent specification.--skip-init,--deploy-run-init, and--from-planflags in Terraform commands.Bug Fixes
Documentation