fix: allow empty env var for moduleVersion as can be set in the moduleSource #190
Conversation
There was a problem hiding this comment.
Pull request overview
This PR makes the MODULE_VERSION environment variable optional in the terraform-module-promise stage, allowing users to specify version information directly in the MODULE_SOURCE URL using the ?ref= query parameter instead.
Key changes:
- Changed
MODULE_VERSIONfrom a required environment variable to an optional one - Updated source URL construction logic to conditionally append version information
- Added test coverage for the scenario where version is specified in
MODULE_SOURCErather thanMODULE_VERSION
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| stages/terraform-module-promise/main.go | Changed MODULE_VERSION from MustHaveEnv to os.Getenv and updated source string construction to conditionally include version query parameter |
| stages/terraform-module-promise/test/stage_test.go | Added new test case verifying that the code works when MODULE_VERSION is not set and version is specified in MODULE_SOURCE |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| version := "" | ||
| if moduleVersion != "" { | ||
| version = fmt.Sprintf("?ref=%s", moduleVersion) | ||
| } | ||
|
|
||
| source := fmt.Sprintf("%s//%s%s", moduleSource, modulePath, version) | ||
| if modulePath == "" { | ||
| source = fmt.Sprintf("%s?ref=%s", moduleSource, moduleVersion) | ||
| source = fmt.Sprintf("%s%s", moduleSource, version) | ||
| } |
There was a problem hiding this comment.
The current implementation can create invalid URLs if both MODULE_SOURCE already contains query parameters (e.g., "?ref=1.0.0") and MODULE_VERSION is also set. This would result in duplicate query parameters like "git::example.com?ref=1.0.0?ref=2.0.0".
Consider validating that MODULE_SOURCE doesn't already contain a ref parameter when MODULE_VERSION is provided, or document that MODULE_VERSION should not be used when MODULE_SOURCE already includes version information. Alternatively, you could parse the URL and detect existing query parameters to prevent conflicts.
| var _ = Describe("Different variable inputs", func() { | ||
| var ( | ||
| envVars map[string]string | ||
| tmpDir string | ||
| ) | ||
|
|
||
| BeforeEach(func() { | ||
| var err error | ||
| tmpDir, err = os.MkdirTemp("", "kratix") | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| }) | ||
|
|
||
| AfterEach(func() { | ||
| os.RemoveAll(tmpDir) | ||
| }) | ||
|
|
||
| It("No version provided", func() { | ||
| envVars = map[string]string{ | ||
| "KRATIX_INPUT_FILE": "assets/test-object.yaml", | ||
| "KRATIX_OUTPUT_DIR": tmpDir, | ||
| "MODULE_SOURCE": "git::example.com?ref=1.0.0", | ||
| } | ||
|
|
||
| session := runWithEnv(envVars) | ||
| Eventually(session).Should(gexec.Exit()) | ||
| Expect(session.Buffer()).To(gbytes.Say("Terraform JSON configuration written to %s/testobject_non-default_test-object.tf.json", tmpDir)) | ||
| Expect(session).To(gexec.Exit(0)) | ||
| output, err := os.ReadFile(filepath.Join(tmpDir, "testobject_non-default_test-object.tf.json")) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(string(output)).To(MatchJSON(expectedOutput)) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
The test only covers the scenario where MODULE_SOURCE contains the ref parameter and MODULE_VERSION is not set. However, there are other important scenarios that should be tested to ensure the change works correctly:
- MODULE_PATH is set but MODULE_VERSION is empty (to ensure the double-slash doesn't cause issues)
- Both MODULE_VERSION is set AND MODULE_SOURCE contains ?ref= (to verify this edge case is handled or at least documented)
- MODULE_VERSION is set with MODULE_PATH (the original happy path with path but no version in source)
These additional test cases would help ensure the implementation handles all expected combinations of input variables.
| os.RemoveAll(tmpDir) | ||
| }) | ||
|
|
||
| It("No version provided", func() { |
There was a problem hiding this comment.
The test description "No version provided" is slightly misleading because a version is actually provided in the MODULE_SOURCE as "?ref=1.0.0". A more accurate description would be "No MODULE_VERSION env var provided" or "Version specified in MODULE_SOURCE" to clarify that the version comes from the source URL rather than the environment variable.
| It("No version provided", func() { | |
| It("No MODULE_VERSION env var provided", func() { |
In moving to enabling registries from the CLI, we now no longer can require moduleVersion in our stage image either.
See #189 as another location where this has popped up.