feat: support credential tokens for getter #4047
Conversation
dlundgren
requested review from
denis256,
levkohimins and
yhakbar
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis pull request introduces a new function, Changes
Assessment against linked issues
Suggested reviewers
✨ Finishing Touches
🪧 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
Documentation and Community
|
|
Thanks for submitting this, @dlundgren . Would you mind adding an integration test that confirms this, but is ignored unless a build flag is provided? That would confirm that we can do automated testing to determine regression if we hook it up to a private GitLab instance, or that someone contributing features related to a private GitLab instance can test for regression locally. |
|
Thanks for the pointer on the test! I was trying to figure out how to add one, but got tripped up on the private registry part of it. |
|
@dlundgren , we're happy to work with you on resolving this. It seems like a good implementation of the feature request. I've moved the PR in-draft until you get a chance to add those tests. Please mark it as ready for review once we can take a look. |
|
I've added the I cloned yorinasub17 terraform-null-terragrunt-registry-test to my gitlab-ce instance, and published the module. I noticed the same repo also lives in gruntwork-io right after I committed this. The It also will require two env variables:
|
![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
🧹 Nitpick comments (6)
test/integration_private_registry_test.go (6)
48-48: Remove unnecessaryt.Helper()callThe
t.Helper()function should only be used in helper functions that are called by test functions, not in the test functions themselves. It helps with reporting the correct file and line number when a test fails.func TestPrivateRegistryWithConfgFileToken(t *testing.T) { - t.Helper()
52-52: Remove or replace debug output to stdoutWriting directly to stdout in tests can interfere with the test runner's output. If you need to debug, consider using
t.Logf()instead.- os.Stdout.Write([]byte(host)) + t.Logf("Using registry host: %s", host)
68-68: Remove unnecessaryt.Helper()callSame issue as in the previous test function -
t.Helper()is meant for helper functions, not test functions.func TestPrivateRegistryWithEnvToken(t *testing.T) { - t.Helper()
72-73: Improve host name transformation for environment variablesThe current transformation replaces dots and hyphens, but Terraform's environment variable naming convention might require additional transformations for other special characters. Consider using a more comprehensive approach.
- host = strings.ReplaceAll(strings.ReplaceAll(host, ".", "_"), "-", "__") + // Convert host to format suitable for Terraform env vars (replace all non-alphanumeric with underscore) + host = strings.ToUpper(host) + var result strings.Builder + for _, c := range host { + if (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') { + result.WriteRune(c) + } else { + result.WriteRune('_') + } + } + host = result.String() t.Setenv(fmt.Sprintf("TF_TOKEN_%s", host), token)
61-64: Add a comment explaining the test's success criteriaThe test expects a specific error about "hashicorp/null", which is an unusual pattern. A comment explaining why this indicates success would improve maintainability.
_, _, err := helpers.RunTerragruntCommandWithOutput(t, "terragrunt init --non-interactive --log-level=trace --working-dir="+rootPath) - // the hashicorp/null provider errors on install, but that indicates that the private tfr module was downloaded + // The test is considered successful if we get an error specifically about the hashicorp/null provider. + // This indicates that authentication worked correctly and we were able to download the private module, + // but then failed at a later step when trying to install the null provider. require.Contains(t, err.Error(), "hashicorp/null", "Error accessing the private registry")
21-45: Add better documentation and consider refactoring the setup functionThe setup function is quite complex and would benefit from more documentation. It also performs multiple responsibilities: environment checking, test setup, URL validation, and file manipulation.
Consider splitting it into smaller, more focused functions:
- A function to check and validate environment variables
- A function to set up the test environment
- A function to customize the test files
This would improve maintainability and testability of the code.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
test/fixtures/private-registry/env.tfrc(1 hunks)test/fixtures/private-registry/terragrunt.hcl(1 hunks)test/integration_private_registry_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- test/fixtures/private-registry/env.tfrc
- test/fixtures/private-registry/terragrunt.hcl
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
**/*.go: Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
test/integration_private_registry_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/integration_private_registry_test.go (2)
42-44: Consider combining URL validation checks.This hostname check is separate from the URL parsing error check above. While this works as defensive programming, you could simplify by combining these checks.
URL, err := url.Parse("tfr://" + registryUrl) - if err != nil { - t.Fatalf("REGISTRY_URL is invalid: %f", err) - } - - if URL.Hostname() == "" { - t.Fatal("REGISTRY_URL is invalid") - } + if err != nil || URL.Hostname() == "" { + t.Fatalf("REGISTRY_URL is invalid: %v", err) + }
65-66: Consider enhancing test assertion messages.While the test logic is correct, the error message could be more descriptive about why seeing "hashicorp/null" in the error output indicates success.
- require.Contains(t, err.Error(), "hashicorp/null", "Error accessing the private registry") + require.Contains(t, err.Error(), "hashicorp/null", "Expected error about hashicorp/null provider, which indicates successful private module download")Also applies to: 80-82
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/integration_private_registry_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
**/*.go: Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
test/integration_private_registry_test.go
🔇 Additional comments (4)
test/integration_private_registry_test.go (4)
53-67: LGTM: Test configuration file token implementation.The test correctly verifies that Terragrunt can authenticate with a private registry using credentials from a configuration file.
69-83: LGTM: Test environment variable token implementation.The test properly validates the environment variable-based authentication method, including the host name transformation logic that matches the implementation.
1-20: LGTM: File structure and build constraints.The build constraint properly isolates these tests to run only when the
private_registrytag is specified, and the imports and constants are well-organized.
21-51: LGTM: Test setup function implementation.The setup function correctly initializes the test environment, validates required environment variables, and prepares the test directory structure. The conditional check for skipping tests now correctly uses the OR operator as previously suggested.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/integration_private_registry_test.go (3)
37-44: Improve URL validation logic and error messagesThe URL validation logic is currently split into two separate checks - one for parsing errors and one for empty hostname. Consider consolidating these checks for better readability and providing more descriptive error messages.
URL, err := url.Parse("tfr://" + registryUrl) - if err != nil { - t.Fatalf("REGISTRY_URL is invalid: %v", err) - } - - if URL.Hostname() == "" { - t.Fatal("REGISTRY_URL is invalid") + if err != nil || URL.Hostname() == "" { + t.Fatalf("REGISTRY_URL is invalid: %v. Expected a valid hostname.", err) }
63-67: Extract duplicate test assertion logic to a helper functionBoth test functions contain nearly identical code for running the Terragrunt command and checking the error message. Consider extracting this to a helper function to reduce duplication.
+func runTerragruntInitAndVerify(t *testing.T, rootPath string) { + t.Helper() + _, _, err := helpers.RunTerragruntCommandWithOutput(t, "terragrunt init --non-interactive --log-level=trace --working-dir="+rootPath) + + // the hashicorp/null provider errors on install, but that indicates that the private tfr module was downloaded + require.Contains(t, err.Error(), "hashicorp/null", "Error accessing the private registry") +} func TestPrivateRegistryWithConfgFileToken(t *testing.T) { // ... existing setup code ... t.Setenv("TF_CLI_CONFIG_FILE", util.JoinPath(rootPath, "env.tfrc")) - - _, _, err := helpers.RunTerragruntCommandWithOutput(t, "terragrunt init --non-interactive --log-level=trace --working-dir="+rootPath) - - // the hashicorp/null provider errors on install, but that indicates that the private tfr module was downloaded - require.Contains(t, err.Error(), "hashicorp/null", "Error accessing the private registry") + runTerragruntInitAndVerify(t, rootPath) } func TestPrivateRegistryWithEnvToken(t *testing.T) { // ... existing setup code ... t.Setenv(fmt.Sprintf("TF_TOKEN_%s", host), token) - - _, _, err := helpers.RunTerragruntCommandWithOutput(t, "terragrunt init --non-interactive --log-level=trace --working-dir="+rootPath) - - // The main test is for authentication against the private registry, so if the null provider fails then we know - // that terragrunt authenticated and downloaded the module. - require.Contains(t, err.Error(), "hashicorp/null", "Error accessing the private registry") + runTerragruntInitAndVerify(t, rootPath) }Also applies to: 78-83
26-27: Enhance documentation about the recommended repository cloneThe comment mentions a recommended repository clone but doesn't explain why this particular repository is recommended or what its purpose is. Consider adding more detail to help other developers understand the test requirements better.
- // the private registry test is recommended to be a clone of gruntwork-io/terraform-null-terragrunt-registry-test + // The private registry test expects a Terraform registry module similar to + // gruntwork-io/terraform-null-terragrunt-registry-test, which is a minimal + // module that references the hashicorp/null provider. This allows us to test + // authentication without needing the module to actually work completely.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/integration_private_registry_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
**/*.go: Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
test/integration_private_registry_test.go
🔇 Additional comments (1)
test/integration_private_registry_test.go (1)
1-84: LGTM! The integration test implementation looks solid.The test structure and implementation effectively validate both authentication methods for private Terraform registries:
- Using credentials from a config file via TF_CLI_CONFIG_FILE
- Using credentials from environment variables
The test appropriately handles environment setup, validation, and assertions. The skipping logic for when credentials aren't available is also correctly implemented.
Description
Fixes #1771 by using the
cliconfigto get the credentials. I've tested with~/.terraformrcand the credentials worked as expected for a private Gitlab instance.TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Updated source downloads to honor
.terraformrccredentialsSummary by CodeRabbit
New Features
Tests