Add component tests #16
Conversation
WalkthroughThis change introduces conditional resource creation in Terraform by adding a Changes
Sequence Diagram(s)sequenceDiagram
participant M as Module
participant T as Terraform Engine
participant K as Kubernetes API
M->>T: Evaluate module.this.enabled flag
alt enabled == true
T->>K: Create kubernetes_ingress_class_v1 resource
T->>K: Create kubernetes_manifest resource
else enabled == false
T-->>K: Skip resource creation
end
sequenceDiagram
participant TS as Test Suite
participant DM as Deployment Manager
participant EK as EKS Cluster API
TS->>DM: Destroy any existing Atmos Component
TS->>DM: Deploy Atmos Component
TS->>EK: Retrieve EKS Cluster ID
EK-->>TS: Return Cluster ID
TS->>EK: Initialize Kubernetes Clientset
EK-->>TS: Return Clientset
TS->>EK: Fetch Ingress Class details
EK-->>TS: Return Ingress Class data
Possibly related issues
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 (
|
|
/terratest |
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
test/fixtures/stacks/catalog/usecase/disabled.yaml (1)
1-12: Disabled Configuration File Review
The new disabled use-case configuration is well structured. The file correctly setsenabled: falsealong with other parameters (e.g.,kube_exec_auth_role_arn_enabled,class_name,group, etc.), which should ensure that no resources are created when this configuration is applied. Please ensure that your component tests verify this behavior.test/go.mod (1)
17-17: Go Module Replace Directive Review
It appears that the replace directive is currently commented out:-// 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.0If this module replacement is intended for production or testing builds, please remove the comment markers so that the directive is active.
test/fixtures/stacks/catalog/eks-cluster.yaml (1)
1-94: EKS Cluster Configuration File Review
This comprehensive configuration file for the EKS cluster component covers a wide spectrum of settings, including node group configuration, addon setups, and access controls. The detailed defaults are beneficial for robust deployment. One note: YAMLlint has flagged a missing newline at the end of the file—please add one to ensure compliance with YAML formatting guidelines.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 94-94: no new line character at the end of file
(new-line-at-end-of-file)
test/component_test.go (3)
11-11: Remove commented-out importThere's an imported but commented-out package. It's better to remove unused imports to keep the code clean.
- // "github.com/gruntwork-io/terratest/modules/aws"
27-28: Consider using camelCase for variable namesThe variables
class_nameandgroup_nameuse snake_case, while Go conventions typically use camelCase. Consider renaming for consistency with Go style conventions.- class_name := fmt.Sprintf("alb-%s", randomID) - group_name := fmt.Sprintf("group-%s", randomID) + className := fmt.Sprintf("alb-%s", randomID) + groupName := fmt.Sprintf("group-%s", randomID)Remember to update the references to these variables in the inputs map as well.
56-60: Consider expanding the disabled component testThe
TestEnabledFlagtest is minimal compared toTestBasicand just callsVerifyEnabledFlag. Consider expanding this test to verify more explicitly that no Kubernetes resources are created when the component is disabled.func (s *ComponentSuite) TestEnabledFlag() { const component = "eks/alb-controller-ingress-class/disabled" const stack = "default-test" + + // First verify the enabled flag s.VerifyEnabledFlag(component, stack, nil) + + // Deploy the component with enabled=false + inputs := map[string]interface{}{ + "enabled": false, + "class_name": "alb-disabled-test", + } + + defer s.DestroyAtmosComponent(s.T(), component, stack, &inputs) + options, err := s.DeployAtmosComponent(s.T(), component, stack, &inputs) + assert.NoError(s.T(), err) + + // Verify that the Kubernetes resource does not exist + clusterOptions := s.GetAtmosOptions("eks/cluster", stack, nil) + clusterId := atmos.Output(s.T(), clusterOptions, "eks_cluster_id") + cluster := awsHelper.GetEksCluster(s.T(), context.Background(), "us-east-2", clusterId) + clientset, err := awsHelper.NewK8SClientset(cluster) + assert.NoError(s.T(), err) + + // Attempt to get the ingress class - it should not exist + _, err = clientset.NetworkingV1().IngressClasses().Get(context.Background(), "alb-disabled-test", metav1.GetOptions{}) + assert.Error(s.T(), err, "Ingress class should not exist when component is disabled") }
📜 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 (16)
src/main.tf(2 hunks)src/provider-helm.tf(1 hunks)src/remote-state.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/eks-cluster.yaml(1 hunks)test/fixtures/stacks/catalog/usecase/basic.yaml(1 hunks)test/fixtures/stacks/catalog/usecase/disabled.yaml(1 hunks)test/fixtures/stacks/catalog/vpc.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
🪛 YAMLlint (1.35.1)
test/fixtures/stacks/catalog/eks-cluster.yaml
[error] 94-94: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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 (17)
src/remote-state.tf (1)
1-4: Module Version Update: Verify Compatibility
The module version has been bumped to"1.8.0"which may include breaking changes or new features compared to the previous version. Please review the module's changelog and test the integration to ensure that the new version works as expected with the rest of the configuration.test/.gitignore (1)
1-6: .gitignore Additions are Clear
The new.gitignorefile in thetestdirectory properly excludes test state, cache files, and temporary testing artifacts. This will help keep your repository clean.test/fixtures/stacks/catalog/vpc.yaml (1)
1-20: New VPC Test Configuration File
This YAML file introduces the VPC component configuration with clear metadata and variable definitions. Verify that the provided values (like availability zones"b"and"c") match the intended test scenarios and expected Terraform inputs. Otherwise, the configuration looks well-structured and ready for component tests.test/fixtures/stacks/catalog/usecase/basic.yaml (1)
1-13: Basic Component Test Configuration Added
This configuration file defines a basic test case for the EKS ALB controller ingress class. The parameters such asenabled: true,class_name: special, andscheme: internet-facingare clearly specified. Ensure that these settings accurately reflect the intended test conditions for verifying basic component functionality.test/fixtures/stacks/orgs/default/test/tests.yaml (1)
1-7: Comprehensive Test Imports Defined
The newtests.yamlfile correctly imports the required test configurations including defaults, VPC, EKS cluster, and both basic and disabled use cases. Please double-check that all referenced configurations (e.g.,catalog/eks-clusterandcatalog/usecase/disabled) exist and integrate properly with the overall test suite.src/main.tf (2)
2-2: Conditional Resource Creation in Ingress Class
The addition ofcount = module.this.enabled ? 1 : 0for thekubernetes_ingress_class_v1resource is a sound approach to conditionally manage resource creation. Make sure thatmodule.this.enabledis correctly defined and that both enabled and disabled scenarios are covered by your tests.
24-24: Conditional Manifest Creation
Similarly, addingcount = module.this.enabled ? 1 : 0to thekubernetes_manifestresource keeps the behavior consistent. Confirm through tests that when the module is disabled, this manifest is not applied.test/fixtures/vendor.yaml (1)
1-43: Vendor Configuration File Review
The newAtmosVendorConfigfile is structured well with clear metadata and comprehensive inclusion/exclusion rules. The templating syntax for the source reference (?ref={{.Version}}) should integrate properly with your CI/CD processes. Please double-check that the templated values are substituted as expected during vendor resolution.test/fixtures/stacks/catalog/account-map.yaml (2)
23-46: Well-structured backend configuration for testsThe static backend configuration is well-designed for testing purposes, allowing tests to use the account map IAM roles module to find the role to assume for Terraform operations.
41-46:✅ Verification successful
Important: TEST_ACCOUNT_ID environment variable must be set correctly
This configuration requires the environment variable
TEST_ACCOUNT_IDto be set properly for tests to work. Without it, the tests won't be able to authenticate correctly.Make sure this environment variable is set in your CI/CD pipeline or test environment:
🏁 Script executed:
#!/bin/bash # Check if TEST_ACCOUNT_ID is set if [[ -z "${TEST_ACCOUNT_ID}" ]]; then echo "Warning: TEST_ACCOUNT_ID environment variable is not set" echo "Tests will use the placeholder value: <TEST_ACCOUNT_ID>" fiLength of output: 257
Action Required: Ensure TEST_ACCOUNT_ID Is Properly Set in Your Test Environment
The configuration in
test/fixtures/stacks/catalog/account-map.yaml(lines 41-46) relies on theTEST_ACCOUNT_IDenvironment variable for proper test authentication. The verification script confirmed that when this variable is unset, a warning is emitted and a placeholder value is used. Please ensure that your CI/CD pipeline or test environment sets a validTEST_ACCOUNT_IDto avoid authentication issues during test runs.
- The provided shell snippet correctly warns if
TEST_ACCOUNT_IDis not set.- Update your CI/CD configuration accordingly to inject the valid environment variable.
src/provider-helm.tf (1)
136-138: Good refactoring: inline evaluation of role ARNThe code has been improved by directly using the
coalesce(var.kube_exec_auth_role_arn, module.iam_roles.terraform_role_arn)expression in theexec_rolelocal variable, rather than storing it in a separate local variable first. This simplifies the code without changing the functionality.test/fixtures/stacks/orgs/default/test/_defaults.yaml (2)
4-35: Good configuration for Terraform backend and variablesThe configuration for the Terraform backend and variables is well-structured, with clear definitions for namespace, tenant, environment, region, stage, and label order.
45-46:❓ Verification inconclusive
Verify TEST_ACCOUNT_ID environment variable
This configuration requires the
TEST_ACCOUNT_IDenvironment variable to be set correctly. If it's not set, it will default to the placeholder<TEST_ACCOUNT_ID>which would cause authentication failures.Ensure this is properly set in test environments:
🏁 Script executed:
#!/bin/bash # Check if TEST_ACCOUNT_ID is set properly if [[ -z "${TEST_ACCOUNT_ID}" || "${TEST_ACCOUNT_ID}" == "<TEST_ACCOUNT_ID>" ]]; then echo "Error: TEST_ACCOUNT_ID environment variable is not set or has default value" echo "Please set it to a valid AWS account ID" exit 1 fiLength of output: 272
Attention: Verify TEST_ACCOUNT_ID in your test environment
The configuration in
test/fixtures/stacks/orgs/default/test/_defaults.yaml
(at lines 45–46) is set to use the value of the environment variableTEST_ACCOUNT_ID. If it isn’t provided, it defaults to the placeholder<TEST_ACCOUNT_ID>, which will cause authentication failures. Our verification script confirmed that whenTEST_ACCOUNT_IDis missing (or still set to<TEST_ACCOUNT_ID>), the check triggers an error message and exits.• Please ensure that your test environments set
TEST_ACCOUNT_IDto a valid AWS account ID.
• If using automated tests, double-check that the required environment variable is properly injected.test/fixtures/atmos.yaml (1)
1-78: Well-documented Atmos CLI configurationThe Atmos CLI configuration file is comprehensive and well-documented with comments explaining each section, making it clear how the configuration is loaded and which settings can be overridden by environment variables or command-line arguments.
test/component_test.go (3)
21-54: Well-structured basic test for EKS ALB controller ingress classThe
TestBasicmethod is comprehensive and thoroughly tests the component by:
- Setting up input parameters with randomized values
- Deploying and later destroying the component
- Verifying the Kubernetes resources are created correctly
- Performing a drift test
This is a well-designed test that covers the essential functionality.
62-66: Good setup for the test suiteThe
SetupSuitemethod properly initializes the test configuration and sets the component destination directory before calling the parent setup method.
68-73: Well-organized test runner with dependenciesThe
TestRunSuitefunction properly sets up the component suite with its dependencies (VPC and EKS cluster) before running the tests. This ensures that the necessary infrastructure is in place before testing the ALB controller ingress class.
|
/terratest |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/component_test.go (1)
37-39:⚠️ Potential issueAdd error handling for DeployAtmosComponent
The error from
DeployAtmosComponentis being ignored, which is not ideal for a test. You should assert that no error occurred.- defer s.DestroyAtmosComponent(s.T(), component, stack, &inputs) - options, _ := s.DeployAtmosComponent(s.T(), component, stack, &inputs) - assert.NotNil(s.T(), options) + defer s.DestroyAtmosComponent(s.T(), component, stack, &inputs) + options, err := s.DeployAtmosComponent(s.T(), component, stack, &inputs) + assert.NoError(s.T(), err) + assert.NotNil(s.T(), options)
🧹 Nitpick comments (2)
test/fixtures/vendor.yaml (1)
1-54: Vendor configuration follows best practicesThe AtmosVendorConfig is well structured with clear component definitions, versioning, and path specifications. All necessary components for testing (account-map, eks/cluster, eks/alb-controller, vpc) are included with specific versions.
Consider setting up automated version checks to ensure these dependencies stay current over time.
test/component_test.go (1)
42-43: Fix typo in variable nameThere's a typo in the variable name
clusrerId(should beclusterId):- clusrerId := atmos.Output(s.T(), clusterOptions, "eks_cluster_id") - cluster := awsHelper.GetEksCluster(s.T(), context.Background(), awsRegion, clusrerId) + clusterId := atmos.Output(s.T(), clusterOptions, "eks_cluster_id") + cluster := awsHelper.GetEksCluster(s.T(), context.Background(), awsRegion, clusterId)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
test/component_test.go(1 hunks)test/fixtures/stacks/catalog/eks-alb-controller.yaml(1 hunks)test/fixtures/stacks/orgs/default/test/tests.yaml(1 hunks)test/fixtures/vendor.yaml(1 hunks)
⏰ 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 (10)
test/fixtures/stacks/orgs/default/test/tests.yaml (1)
1-7: LGTM! Import configuration looks well structured.The imports align perfectly with the PR objectives by including the necessary configurations for basic tests, disabled state tests, and component testing. The structure follows the standard pattern for organizing test fixture stacks.
test/fixtures/stacks/catalog/eks-alb-controller.yaml (4)
11-13: Great documentation on chart version dependency!The comment about updating the IAM policy when changing the chart version is valuable information that helps prevent misconfigurations during future updates.
16-18: Properly documented experiment feature flagGood job noting the issue with the experiment feature by including both the description of the problem and a reference to the GitHub issue for further investigation.
23-29: Well-documented configuration pattern for chart_valuesThe example of how to use
chart_valueshelps future developers understand how to extend the configuration without modifying the component code.
30-36: Resource limits are appropriately setThe resource limits and requests are set to reasonable values for this component, which helps with cluster resource management. The CPU and memory settings follow best practices for Kubernetes deployments.
test/component_test.go (5)
48-52: Good validation of the created resourcesThe test correctly validates that the IngressClass was created with the expected name and controller. This confirms that the component is working as intended.
53-53: Excellent implementation of drift testingIncluding a drift test is an important part of validating Terraform component stability. This ensures the component maintains its state without drifting during subsequent runs with identical inputs, which was one of the key objectives of this PR.
56-60: Good implementation of enabled flag testingThe
TestEnabledFlagmethod checks that no resources are created when the component is disabled, which addresses one of the core objectives of this PR. Using the helper methodVerifyEnabledFlagsimplifies the implementation.
62-66: Well-structured test suite setupThe
SetupSuitemethod properly initializes the configuration and sets the component destination directory, which ensures the tests are running against the correct component.
68-74: Dependencies are clearly definedThe test setup adds all required dependencies (vpc, eks/cluster, eks/alb-controller) before running the suite, which ensures the test environment is properly configured.
|
These changes were released in v1.535.1. |
What
basiccomponent testdisabledcomponent testWhy
enabled: falsesetReferences
Summary by CodeRabbit
New Features
Tests
Chores