Skip to content

Conversation

@goruha
Copy link
Contributor

@goruha goruha commented Oct 21, 2025

what

  • Added tests

why

  • Verify that component works for opentofu and terraform versions

Summary by CodeRabbit

  • New Features

    • Enhanced password security with minimum character requirements for Elasticsearch deployments.
    • Expanded test coverage for Elasticsearch component configurations.
  • Chores

    • Updated test infrastructure and dependency configurations.
    • Refined build artifact exclusions.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 21, 2025

Walkthrough

Adds test infrastructure and fixtures for Elasticsearch, updates a Terraform random_password resource to support conditional creation and stricter password constraints, and introduces a Go test suite plus Atmos/vendor manifests and stack fixtures.

Changes

Cohort / File(s) Change Summary
Terraform Resource Enhancement
src/main.tf
Added conditional creation via count = local.create_password ? 1 : 0; renamed number = truenumeric = true; added min_lower = 1 and min_numeric = 1.
Go Test Suite
test/component_test.go
New test file: ComponentSuite type with embedded helper.TestSuite, TestBasic() method exercising Elasticsearch component, and TestRunSuite() entrypoint.
Test Ignore Patterns
test/.gitignore
Added entries: state/, .cache, test/test-suite.json, .atmos, test_suite.yaml.
Atmos & Module Config
test/fixtures/atmos.yaml, test/go.mod
Added Atmos CLI config (components, stacks, workflows, logs, templates) and a new Go module manifest with dependencies.
Catalog Component Fixtures
test/fixtures/stacks/catalog/account-map.yaml, .../dns-primary.yaml, .../dns-delegated.yaml, .../vpc.yaml
Added component fixtures for account-map, dns-primary, dns-delegated, and vpc with metadata and vars.
Elasticsearch Use-Case Fixtures
test/fixtures/stacks/catalog/usecase/basic.yaml, .../usecase/disabled.yaml
Added enabled and disabled Elasticsearch component fixtures (instance settings, encryption, master/node options, hostnames).
Stack Defaults & Imports
test/fixtures/stacks/orgs/default/test/_defaults.yaml, .../tests.yaml
Added default backend/vars/account-map mappings and import manifest referencing catalog fixtures.
Vendor Manifest & Cleanup
test/fixtures/vendor.yaml, (deleted) test/run.sh
Added Atmos vendor manifest referencing four external Terraform sources; removed legacy test/run.sh.

Sequence Diagram(s)

sequenceDiagram
  participant Runner as Test Runner
  participant Suite as ComponentSuite
  participant TF as Terraform
  participant ES as Elasticsearch Component
  participant Verifier as Output Verifier

  Runner->>Suite: Run TestRunSuite()
  Suite->>TF: terraform init/apply (use fixtures)
  TF->>ES: create resources (includes optional random_password)
  alt local.create_password == true
    TF->>TF: random_password created (numeric,min_lower,min_numeric)
  else local.create_password == false
    Note right of TF: random_password not created (count = 0)
  end
  TF->>Suite: outputs (domain, endpoints, IAM, SSM keys)
  Suite->>Verifier: validate outputs & drift test
  Verifier-->>Suite: pass/fail
  Suite-->>Runner: test result
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hopped through YAML beds at dawn,

Passwords born only if a flag is on.
Fixtures planted, tests take flight,
Elasticsearch sleeps snug at night,
A rabbit cheers: "Build and run!" 🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "Added tests" is partially related to the changeset. The pull request does indeed add comprehensive test infrastructure, including a new Go test file (component_test.go) with a ComponentSuite type and TestBasic method, multiple test fixture files (YAML configurations for Atmos, stacks, and catalogs), and a Go module manifest. The title accurately reflects this primary objective stated in the PR description ("what: Added tests"). However, the title could be more specific about what is being tested (Elasticsearch component) or more fully capture the scope of changes, including the functional modifications to src/main.tf (conditional password creation, attribute renaming, and minimum constraints) that enable the test infrastructure.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch added-tests

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cf95633 and 1f302fb.

📒 Files selected for processing (1)
  • src/main.tf (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/{main,variables,outputs,providers,versions,context}.tf

📄 CodeRabbit inference engine (AGENTS.md)

Keep the Terraform component source of truth in src/: main.tf, variables.tf, outputs.tf, providers.tf, versions.tf, context.tf

Files:

  • src/main.tf
**/*.tf

📄 CodeRabbit inference engine (AGENTS.md)

**/*.tf: Use 2-space indentation for Terraform files
Prefer lower_snake_case for Terraform variables and locals; keep resource/data names descriptive and aligned with Cloud Posse null-label patterns
Ensure Terraform files are formatted (terraform fmt -recursive) and contain no formatting violations
Comply with TFLint rules configured in .tflint.hcl; do not commit lint violations

Files:

  • src/main.tf
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Summary
🔇 Additional comments (3)
src/main.tf (3)

14-17: Solid conditional password creation logic.

The locals correctly implement a fallback pattern: generate a password only if the component is enabled and no external password is provided. The splat syntax at line 16 safely handles the conditional resource count.


59-73: No issues found. The random provider constraint is correctly specified.

The output confirms that versions.tf already specifies version = ">= 3.0" for the random provider, which properly supports the numeric = true attribute used in src/main.tf. The version constraint is correct and prevents apply failures on older provider versions.


60-60: I need to examine the password constraints defined in the code and check if they're validated.

Now let me examine the actual password resource definition in the code:

Password constraints are enforced by Terraform's random provider, not Elasticsearch—this review comment is based on a false assumption.

The original comment asks to "confirm that the Elasticsearch module and test fixtures validate these constraints are correctly enforced during deployment." However, this premise is incorrect:

  1. Terraform's random_password resource validates at apply time that the password length meets the constraint: length >= (min_upper + min_lower + min_numeric + min_special)
  2. Elasticsearch does not enforce character composition constraints; it only requires a password to exist
  3. The test output correctly validates only the SSM key path, as password structure validation is a Terraform provider responsibility that occurs at resource creation time

The password constraints (min_lower=1, min_numeric=1, min_special=1, min_upper=1) with a 33-character password are appropriate and are enforced by Terraform's cryptographic random provider—not by Elasticsearch or test fixtures.

Likely an incorrect or invalid review comment.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@mergify mergify bot requested review from a team October 21, 2025 23:55
@mergify mergify bot added triage Needs triage needs-test Needs testing labels Oct 21, 2025
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (11)
test/go.mod (2)

1-3: Use a unique module path and pin toolchain (optional but recommended)

“module test” is ambiguous; prefer the repo path. Also consider a toolchain line for reproducibility.

-module test
+module github.com/cloudposse-terraform-components/aws-elasticsearch/test

 go 1.23.0
+
+toolchain go1.23.0

Ensure CI installs Go 1.23.x (or update toolchain to match your runner).


35-35: Remove unnecessary docdb direct dependency

Verification confirms docdb is not actively imported—only a commented-out reference exists in test/component_test.go. The direct dependency in test/go.mod can be safely removed.

-	github.com/aws/aws-sdk-go-v2/service/docdb v1.40.10
src/main.tf (1)

79-80: Incorrect SSM parameter description

The description references Aurora Postgres; this parameter stores the Elasticsearch admin password.

-  description = "Primary Aurora Postgres Password for the master DB user"
+  description = "Elasticsearch admin password for the master user"
test/component_test.go (5)

27-29: Assert NotEmpty for AWS account ID

accountId is a string; NotNil always passes. Use NotEmpty.

-	assert.NotNil(s.T(), accountId)
+	assert.NotEmpty(s.T(), accountId)

33-34: Handle DeployAtmosComponent error

Ignoring the error can mask real failures.

-	options, _ := s.DeployAtmosComponent(s.T(), component, stack, &inputs)
-	assert.NotNil(s.T(), options)
+	options, err := s.DeployAtmosComponent(s.T(), component, stack, &inputs)
+	assert.NoError(s.T(), err)
+	assert.NotNil(s.T(), options)

39-47: Reduce brittle string expectations

Hard-coded prefixes (e.g., “eg-default-ue2-test-e-”) make tests fragile across naming tweaks. Prefer asserting stable ARN/endpoint shapes and substrings.

Example:

-	assert.True(s.T(), strings.HasPrefix(domainArn, fmt.Sprintf("arn:aws:es:%s:%s:domain/eg-default-ue2-test-e-", awsRegion, accountId)))
+	assert.True(s.T(), strings.HasPrefix(domainArn, fmt.Sprintf("arn:aws:es:%s:%s:domain/", awsRegion, accountId)))
+	assert.Contains(s.T(), domainArn, ":domain/")

Apply similar loosening to domainEndpoint/kibanaEndpoint/domainHostname checks.


63-65: Avoid exact match on SSM key; assert prefix/suffix

Exact equality ties the test to the component name. Prefer prefix/suffix.

-	assert.Equal(s.T(), masterPasswordSSMKey, "/elasticsearch/e/password")
+	assert.True(s.T(), strings.HasPrefix(masterPasswordSSMKey, "/elasticsearch/"))
+	assert.True(s.T(), strings.HasSuffix(masterPasswordSSMKey, "/password"))

3-16: Drop commented imports

Remove commented imports to keep the file clean.

-	//"context"
-	// "github.com/aws/aws-sdk-go-v2/service/docdb"
-	// awshelper "github.com/cloudposse/test-helpers/pkg/aws"
test/fixtures/stacks/catalog/account-map.yaml (1)

23-46: Static backend config looks fine; ensure TEST_ACCOUNT_ID is set in CI

The static backend depends on TEST_ACCOUNT_ID for role resolution. Make sure your pipeline exports it.

test/fixtures/vendor.yaml (1)

7-54: Pin sources to immutable SHAs (optional)

Tags can be moved; pinning to commit SHAs improves supply‑chain reproducibility. Keep the tag in a comment for readability.

Example:

source: github.com/cloudposse-terraform-components/aws-dns-delegated.git//src?ref=abcdef1234567890 # v1.535.1
test/fixtures/stacks/orgs/default/test/_defaults.yaml (1)

8-9: Consider absolute or environment-aware paths for Terraform state backend.

The local backend configuration uses relative paths with template defaults. While this works for test fixtures, relative paths can cause issues when the working directory varies across execution contexts. Ensure all test invocations execute from the expected directory, or document this requirement clearly.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 919778e and cf95633.

⛔ Files ignored due to path filters (1)
  • test/go.sum is excluded by !**/*.sum
📒 Files selected for processing (15)
  • src/main.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/dns-delegated.yaml (1 hunks)
  • test/fixtures/stacks/catalog/dns-primary.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
📓 Path-based instructions (6)
test/fixtures/**

📄 CodeRabbit inference engine (AGENTS.md)

Store Atmos-based test scenarios and supporting data under test/fixtures/

Files:

  • test/fixtures/stacks/orgs/default/test/tests.yaml
  • test/fixtures/atmos.yaml
  • test/fixtures/stacks/catalog/account-map.yaml
  • test/fixtures/stacks/catalog/vpc.yaml
  • test/fixtures/stacks/catalog/usecase/disabled.yaml
  • test/fixtures/stacks/catalog/dns-primary.yaml
  • test/fixtures/stacks/orgs/default/test/_defaults.yaml
  • test/fixtures/vendor.yaml
  • test/fixtures/stacks/catalog/dns-delegated.yaml
  • test/fixtures/stacks/catalog/usecase/basic.yaml
**/*.{yml,yaml,md}

📄 CodeRabbit inference engine (AGENTS.md)

Use 2-space indentation for YAML and Markdown files

Files:

  • test/fixtures/stacks/orgs/default/test/tests.yaml
  • test/fixtures/atmos.yaml
  • test/fixtures/stacks/catalog/account-map.yaml
  • test/fixtures/stacks/catalog/vpc.yaml
  • test/fixtures/stacks/catalog/usecase/disabled.yaml
  • test/fixtures/stacks/catalog/dns-primary.yaml
  • test/fixtures/stacks/orgs/default/test/_defaults.yaml
  • test/fixtures/vendor.yaml
  • test/fixtures/stacks/catalog/dns-delegated.yaml
  • test/fixtures/stacks/catalog/usecase/basic.yaml
test/**/*_test.go

📄 CodeRabbit inference engine (AGENTS.md)

Place Go Terratest files under test/ and name them *_test.go

Files:

  • test/component_test.go
test/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

Use Go Terratest with github.com/cloudposse/test-helpers and Atmos fixtures for integration tests

Files:

  • test/component_test.go
src/{main,variables,outputs,providers,versions,context}.tf

📄 CodeRabbit inference engine (AGENTS.md)

Keep the Terraform component source of truth in src/: main.tf, variables.tf, outputs.tf, providers.tf, versions.tf, context.tf

Files:

  • src/main.tf
**/*.tf

📄 CodeRabbit inference engine (AGENTS.md)

**/*.tf: Use 2-space indentation for Terraform files
Prefer lower_snake_case for Terraform variables and locals; keep resource/data names descriptive and aligned with Cloud Posse null-label patterns
Ensure Terraform files are formatted (terraform fmt -recursive) and contain no formatting violations
Comply with TFLint rules configured in .tflint.hcl; do not commit lint violations

Files:

  • src/main.tf
🪛 OSV Scanner (2.2.3)
test/go.mod

[HIGH] 1-1: golang.org/x/crypto 0.32.0: Potential denial of service in golang.org/x/crypto

(GO-2025-3487)


[HIGH] 1-1: golang.org/x/crypto 0.32.0: golang.org/x/crypto Vulnerable to Denial of Service (DoS) via Slow or Incomplete Key Exchange

(GHSA-hcg3-q754-cr77)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Summary
🔇 Additional comments (14)
test/.gitignore (1)

1-5: LGTM!

All entries appropriately exclude test artifacts and Atmos-generated metadata.

test/fixtures/atmos.yaml (1)

1-77: LGTM!

Configuration is comprehensive, well-documented, and appropriate for test infrastructure. Atmos settings, logging, and template configurations are correctly configured.

test/fixtures/stacks/catalog/vpc.yaml (1)

1-19: LGTM!

Test VPC fixture is well-configured for test scenarios with appropriate defaults (public subnets enabled, NAT disabled, flow logs disabled).

test/fixtures/stacks/catalog/dns-primary.yaml (1)

1-9: LGTM!

DNS primary fixture is appropriately configured with test domain and minimal configuration.

test/fixtures/stacks/catalog/dns-delegated.yaml (1)

1-10: LGTM!

DNS delegated fixture properly complements the DNS primary configuration for comprehensive DNS testing.

test/fixtures/stacks/catalog/usecase/basic.yaml (1)

1-18: LGTM!

Elasticsearch basic fixture provides comprehensive configuration for an enabled instance. Note: metadata.component is set to "target"—verify this is the intended generic/placeholder component reference.

test/fixtures/stacks/catalog/usecase/disabled.yaml (1)

1-17: LGTM!

Disabled Elasticsearch fixture correctly mirrors the basic configuration with enabled: false, providing test coverage for the absent Elasticsearch scenario.

test/fixtures/stacks/orgs/default/test/tests.yaml (1)

1-7: LGTM!

Test stack imports are properly organized with defaults first, followed by shared infrastructure (DNS, VPC), and test scenarios. Ensure orgs/default/test/_defaults.yaml is properly configured and available.

src/main.tf (3)

60-60: Conditional password creation LGTM

count = local.create_password ? 1 : 0 prevents creating a secret when one is supplied. Good.


64-67: Attribute rename and charset flags LGTM; consider restricting special characters

numeric=true fixes the provider attribute. If OpenSearch rejects certain symbols, set override_special to an allowed set to avoid failures during domain creation.

Would you like me to propose an override_special set aligned with AWS constraints?


69-73: Minimum character class constraints LGTM

min_lower/min_numeric ensure complexity. No issues.

test/fixtures/stacks/orgs/default/test/_defaults.yaml (3)

44-50: Verify contradictory EKS configuration.

The account has eks: true (line 44) but the tags contain eks: false (line 50). This appears contradictory—confirm whether both are intended or if one should be corrected.


66-66: Verify empty string for terraform_roles is intentional.

The terraform_roles.default-test is set to an empty string. Confirm this is the expected test configuration; if a non-empty role is needed, update accordingly.


1-34: YAML structure and indentation look good.

The import statement, backend configuration, and terraform variables follow correct 2-space indentation and are well-structured for a test fixture.

@goruha
Copy link
Contributor Author

goruha commented Oct 22, 2025

/terratest

1 similar comment
@goruha
Copy link
Contributor Author

goruha commented Oct 22, 2025

/terratest

@mergify mergify bot removed the triage Needs triage label Oct 22, 2025
@mergify mergify bot requested a review from a team October 22, 2025 08:50
@goruha goruha added this pull request to the merge queue Oct 22, 2025
Merged via the queue into main with commit 5c5ec4c Oct 22, 2025
19 checks passed
@goruha goruha deleted the added-tests branch October 22, 2025 13:14
@github-actions
Copy link
Contributor

These changes were released in v1.536.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-test Needs testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants