fix: Ensure Version field is handled correctly when bootstrapping TF module Promise #191
Conversation
aclevername
changed the title
aclevername
changed the title
aclevername
changed the title
There was a problem hiding this comment.
Pull request overview
This PR improves the handling of Terraform module versions when bootstrapping Terraform module Promises. The key change is renaming --module-version to --module-registry-version to clarify that this flag is only applicable to Terraform registry sources, not Git or local module sources.
Key Changes:
- Renamed CLI flag and environment variable from
MODULE_VERSIONtoMODULE_REGISTRY_VERSIONto clarify usage - Added validation to error when
--module-registry-versionis provided with non-registry sources (Git URLs or local paths) - For Git sources, the version/ref is now embedded directly in the
MODULE_SOURCE(e.g.,git::https://...?ref=v1.0.0) - Added comprehensive integration tests covering Git (public and private), registry, nested registry, and local module sources
- Updated CI workflow to support testing against private Git repositories via SSH
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/init_tf_module_promise.go | Renamed flag to --module-registry-version, added validation to reject registry version with non-registry sources, updated help text and examples |
| internal/terraform_module.go | Added IsTerraformRegistrySource() helper function to detect registry vs non-registry sources; conditionally applies version only for registry sources |
| internal/terraform_module_test.go | Added unit tests for registry source detection and registry version handling |
| stages/terraform-module-promise/main.go | Updated to use MODULE_REGISTRY_VERSION env var and validate it's only used with registry sources |
| stages/terraform-module-promise/test/stage_test.go | Updated tests to use MODULE_REGISTRY_VERSION, removed separate MODULE_VERSION env var, added test for error case |
| test/init_tf_module_sources_test.go | New comprehensive integration test covering various module source types (Git, registry, local) |
| test/assets/terraform/modules/local/basic/variables.tf | Test fixture for local module testing |
| test/assets/terraform/api/*.yaml | Test fixture CRD definitions for validating generated schemas from different module types |
| .github/workflows/tests.yaml | Added SSH setup to enable testing against private Git repositories |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| v1alpha1.Container{ | ||
| Name: "terraform-generate", | ||
| Image: "ghcr.io/syntasso/kratix-cli/terraform-generate:v0.2.0", | ||
| Image: "ghcr.io/syntasso/kratix-cli/terraform-generate:v0.4.0", |
There was a problem hiding this comment.
this was never updated to the latest image 0.3.1 😢 pre-empting the next release of this by adding v0.4.0
MODULE_SOURCEenv var from tf stage--module-versionto--module-registry-versionto make the usage clearpaired with @abangser & @codex
closes #189