Add component tests #16
Conversation
WalkthroughThis pull request introduces conditional logic into Terraform configurations, affecting how values are assigned in both resource definitions and outputs based on the Changes
Sequence Diagram(s)sequenceDiagram
participant TF as "Terraform Engine"
participant Main as "main.tf"
participant Outputs as "outputs.tf"
TF->>Main: Evaluate local.enabled
alt local.enabled == true
Main->>Main: Process var.permissions via toset(compact([...]))
Main->>Outputs: Output repository details from local.github_repository
else local.enabled == false
Main->>Main: Set team_slugs to empty list
Main->>Outputs: Output null values in repository outputs
end
Poem
✨ 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
CodeRabbit Configuration File (
|
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟢 Require terratestWonderful, this rule succeeded.This rule require terratest status
|
|
/terratest |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
test/go.mod (1)
17-18: Consider uncommenting the replace directive if neededThe replace directive is commented out. If it's necessary for the tests to function correctly, consider uncommenting it.
-// replace github.com/rebuy-de/aws-nuke/v2/mocks => github.com/ekristen/aws-nuke/v3/ v3.29.0 +replace github.com/rebuy-de/aws-nuke/v2/mocks => github.com/ekristen/aws-nuke/v3/ v3.29.0test/component_test.go (2)
6-8: Remove or use commented importsThere are commented-out imports that should be removed if they're not needed to keep the code clean.
import ( "fmt" "os" - // "strconv" "testing" "strings" helper "github.com/cloudposse/test-helpers/pkg/atmos/component-helper" awsTerratest "github.com/gruntwork-io/terratest/modules/aws" "github.com/gruntwork-io/terratest/modules/random" - // "github.com/jferrl/go-githubauth" "github.com/stretchr/testify/assert" )Also applies to: 13-14
48-54: Remove unused constantThe
awsRegionconstant is defined but never used in this test method.func (s *ComponentSuite) TestEnabledFlag() { const component = "argocd-github-repo/disabled" const stack = "default-test" - const awsRegion = "us-east-2" s.VerifyEnabledFlag(component, stack, nil) }🧰 Tools
🪛 golangci-lint (1.64.8)
51-51: const
awsRegionis unused(unused)
test/fixtures/atmos.yaml (3)
11-18: Clear Base Path Configuration.
Thebase_pathsetting is well-documented with fallback options via environment variables and command-line arguments. Verify that the default empty string ("") behaves as expected across dependent configurations.
34-50: Comprehensive Stacks Configuration.
The stacks block clearly defines the base path, included/excluded paths, and naming patterns. Consider reviewing whether additional validations or defaults are needed for the path arrays to keep stack selection robust.
56-61: Effective Log Settings.
Defining the log file as/dev/stdoutand specifying the log level (Info) are practical defaults. If dynamic log level configuration is later needed, consider exposing an override via environment variables.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
test/go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
src/main.tf(1 hunks)src/outputs.tf(1 hunks)test/.gitignore(1 hunks)test/component_test.go(1 hunks)test/fixtures/atmos.yaml(1 hunks)test/fixtures/stacks/catalog/account-map.yaml(1 hunks)test/fixtures/stacks/catalog/usecase/basic.yaml(1 hunks)test/fixtures/stacks/catalog/usecase/disabled.yaml(1 hunks)test/fixtures/stacks/orgs/default/test/_defaults.yaml(1 hunks)test/fixtures/stacks/orgs/default/test/tests.yaml(1 hunks)test/fixtures/vendor.yaml(1 hunks)test/go.mod(1 hunks)test/run.sh(0 hunks)
💤 Files with no reviewable changes (1)
- test/run.sh
🧰 Additional context used
🪛 golangci-lint (1.64.8)
test/component_test.go
51-51: const awsRegion is unused
(unused)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (22)
test/.gitignore (1)
1-6: .gitignore entries are well-defined.
The new.gitignorefile correctly ignores common temporary and state files (such as thestate/directory,.cache, and test output files) that should not be tracked in version control. Please double-check that these patterns do not conflict with any global or repository-level.gitignoresettings.test/fixtures/stacks/orgs/default/test/tests.yaml (1)
1-5: Import configuration is clear and structured.
The YAML file cleanly imports the defaults and test use cases for both the basic and disabled scenarios. Verify that the imported paths (orgs/default/test/_defaults,catalog/usecase/basic, andcatalog/usecase/disabled) exist and match the intended structure in your repository.test/fixtures/stacks/catalog/account-map.yaml (1)
1-47: Account-map configuration is comprehensive.
This new YAML configuration clearly outlines the component definition for the Terraform account-map, including metadata, variables, and a remote state backend setup. The detailed inline comments provide valuable context regarding the purpose and limitations of the static backend. If future enhancements are needed, consider adding a small example or further documentation for end users.test/fixtures/vendor.yaml (1)
1-19: Atmos Vendor Configuration is clear and appropriately versioned.
The manifest defines the Atmos vendor configuration with a clear source, version, and inclusion patterns for relevant files. Make sure that the component path and version (1.520.0) are up-to-date and continue to align with the expectations in your testing framework.test/fixtures/stacks/catalog/usecase/disabled.yaml (1)
1-23: LGTM: Configuration for disabled test case is well-structuredThe configuration properly sets
enabled: falsewhich is crucial for testing that no resources are created when the component is disabled, aligning perfectly with one of the PR objectives.test/fixtures/stacks/catalog/usecase/basic.yaml (1)
1-23: LGTM: Basic test configuration looks goodThe configuration correctly sets
enabled: trueand provides all necessary parameters for testing the basic functionality of the component, which aligns with the PR objective to test basic features.test/fixtures/stacks/orgs/default/test/_defaults.yaml (1)
1-66: LGTM: Test environment setup looks comprehensiveThe configuration provides all necessary components for a proper test environment, including backend configuration, variables, and account mappings. The use of environment variables for dynamic values is a good practice.
test/component_test.go (2)
21-46: LGTM: Basic test implementation looks goodThe test properly sets up the environment, handles GitHub token securely, ensures proper cleanup using defer, and performs drift testing. This implementation satisfies the PR objective for basic component testing.
48-54: LGTM: Enabled flag test implementation is appropriateThis test verifies that no resources are created when the component is disabled (
enabled: false), which satisfies one of the PR objectives.🧰 Tools
🪛 golangci-lint (1.64.8)
51-51: const
awsRegionis unused(unused)
test/fixtures/atmos.yaml (5)
1-10: Great File Header & Documentation!
The initial comments concisely explain the configuration loading order and support for POSIX-style globs. The documentation here is clear and sets a solid context for users.
20-33: Well-Structured Components Block.
The Terraform components configuration is detailed and includes settings for auto-approval, initialization, and backend file generation. This block is thorough and follows best practices.
51-55: Concise Workflows Configuration.
The workflows block is short and consistent with the structure used in other sections. No issues found.
62-65: Straightforward Settings Block.
The merge strategy for lists is clearly set here. This simplicity aids maintainability.
66-78: Robust Templates Configuration.
The templates section is comprehensive, enabling both Sprig and Gomplate utilities. Links to documentation embedded in the comments enhance the readability and utility for users updating or auditing these options.src/outputs.tf (8)
1-4: Deploy Keys SSM Paths Output.
The output fordeploy_keys_ssm_pathscorrectly referencesmodule.store_write.names. The configuration appears sound and maintains consistency with expected outputs.
6-10: Deploy Keys SSM Path Format Output.
Using the ternary operator to conditionally outputvar.ssm_github_deploy_key_formatwhen enabled (andnullotherwise) is clear and appropriate for managing the state.
11-15: Repository Name Output with Dual Conditionals.
The output forrepositorycombineslocal.enabledandvar.create_repoeffectively—falling back tovar.namewhen necessary. This layered condition ensures that repository naming remains consistent based on the creation flag.
16-19: Improved Repository Description Output.
The change to conditionally returnlocal.github_repository.descriptiononly whenlocal.enabledis true is a smart move for preventing misconfiguration.
21-25: Consistent Repository Default Branch Output.
The updated logic forrepository_default_branchmirrors the pattern seen elsewhere, ensuring consistency. It cleanly prevents exposing branch details when the repository is disabled.
26-30: Conditional Repository URL Output.
Applying the conditionlocal.enabled ? local.github_repository.html_url : nullensures that the repository URL is only output when applicable, which helps avoid leaking stale or irrelevant data.
31-35: Refined Git Clone URL Output.
The output forrepository_git_clone_urlconditionally outputs the URL based onlocal.enabled, maintaining coherence with other conditional outputs.
36-40: Secure SSH Clone URL Output.
The conditional assignment forrepository_ssh_clone_urlhelps ensure that sensitive SSH details are only provided when the repository is enabled, safeguarding against unintended exposure.
|
/terratest |
|
/terratest |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
test/component_test.go (3)
6-7: Remove commented-out imports.There are unused commented imports that should be removed to improve code cleanliness and maintainability.
import ( "fmt" "os" - // "strconv" "testing" "strings" helper "github.com/cloudposse/test-helpers/pkg/atmos/component-helper" awsTerratest "github.com/gruntwork-io/terratest/modules/aws" "github.com/gruntwork-io/terratest/modules/random" - // "github.com/jferrl/go-githubauth" "github.com/stretchr/testify/assert" )Also applies to: 13-13
26-26: Add GitHub token validation.The test retrieves the GitHub token from environment variables but doesn't validate if it exists. Without this check, the test might fail in unexpected ways if the token is missing.
token := os.Getenv("GITHUB_TOKEN") +if token == "" { + s.T().Fatal("GITHUB_TOKEN environment variable must be set") +}
50-54: Remove unused awsRegion constant.The awsRegion constant is defined but never used in this method, as confirmed by static analysis.
func (s *ComponentSuite) TestEnabledFlag() { const component = "argocd-github-repo/disabled" const stack = "default-test" - const awsRegion = "us-east-2" s.VerifyEnabledFlag(component, stack, nil) }🧰 Tools
🪛 golangci-lint (1.64.8)
53-53: const
awsRegionis unused(unused)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/component_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.64.8)
test/component_test.go
53-53: const awsRegion is unused
(unused)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (3)
test/component_test.go (3)
21-48: Well-implemented basic component test.The TestBasic method properly tests the core functionality with appropriate setup and cleanup:
- Creates a randomized resource name for isolation
- Sets up and tears down AWS SSM parameters
- Deploys the component with the required inputs
- Performs a drift test to verify idempotency
This successfully implements the "basic component test" and "drift test" requirements from the PR objectives.
50-56: Properly implemented disabled component test.The TestEnabledFlag method correctly uses the helper's VerifyEnabledFlag method to test that no resources are created when the component is disabled, satisfying the "disabled component test" requirement from the PR objectives.
🧰 Tools
🪛 golangci-lint (1.64.8)
53-53: const
awsRegionis unused(unused)
58-61: Correct test suite setup.The TestRunSuite function properly initializes and runs the test suite using the helper library's Run function, following Go testing best practices.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
test/component_test.go (7)
7-7: Remove the commented-out import.The import for "strconv" is commented out but not used anywhere in the code. Clean up by removing this unused import.
- // "strconv"
28-29: Add validation for required environment variables.The code retrieves the GitHub token from environment variables but doesn't check if it exists. This could lead to cryptic errors later if the token is missing.
token := os.Getenv("GITHUB_TOKEN") + if token == "" { + s.T().Fatal("GITHUB_TOKEN environment variable is required but not set") + }
52-53: Use a custom HTTP client with timeout for GitHub API calls.The default HTTP client used by the GitHub client doesn't have a timeout set, which could lead to hanging tests if the GitHub API is unresponsive.
- client := github.NewClient(nil).WithAuthToken(token) + import "net/http" + import "time" + + httpClient := &http.Client{ + Timeout: time.Second * 30, + } + client := github.NewClient(httpClient).WithAuthToken(token)
69-69: Remove unused constant.The
awsRegionconstant is declared but never used in the TestEnabledFlag method.- const awsRegion = "us-east-2"🧰 Tools
🪛 golangci-lint (1.64.8)
69-69: const
awsRegionis unused(unused)
62-64: Move drift test before GitHub API calls for better test organization.The drift test is performed after the GitHub API tests, which doesn't follow a logical test progression. Consider moving the drift test to be executed immediately after the deployment to maintain a clearer test workflow.
- assert.Nil(s.T(), err) - s.DriftTest(component, stack, &inputs) + assert.Nil(s.T(), err) + + // Verify that there's no drift after deployment + s.DriftTest(component, stack, &inputs)
22-64: Add test for the repository visibility setting.The test creates a GitHub repository but doesn't verify if it's created with the correct visibility setting (public/private). Consider adding an assertion to verify this important property.
assert.NotNil(s.T(), repo) + + // Verify repository visibility (assuming the default is private) + assert.Equal(s.T(), "private", *repo.Visibility, "Repository should be private by default")
41-47: Consider parameterizing test inputs for better testability.Current test inputs are hardcoded in the test function. Consider extracting these into a separate function or struct that can be reused or modified for different test scenarios.
This would make it easier to create variations of the test with different inputs, such as testing with different repository settings or organizations. Consider creating a helper function that returns the test inputs with optional overrides.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
test/go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
test/component_test.go(1 hunks)test/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/go.mod
🧰 Additional context used
🪛 golangci-lint (1.64.8)
test/component_test.go
69-69: const awsRegion is unused
(unused)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (1)
test/component_test.go (1)
49-50: Check for deployment errors.The DeployAtmosComponent call ignores the error return value. This could mask deployment failures and make debugging difficult.
-options, _ := s.DeployAtmosComponent(s.T(), component, stack, &inputs) +options, err := s.DeployAtmosComponent(s.T(), component, stack, &inputs) +if err != nil { + s.T().Fatalf("Failed to deploy component: %v", err) +} assert.NotNil(s.T(), options)
|
/terratest |
|
These changes were released in v1.536.0. |
What
basiccomponent testdisabledcomponent testWhy
enabled: falsesetReferences
Summary by CodeRabbit
New Features
Tests
Chores