Skip to content

fix: allow empty env var for moduleVersion as can be set in the moduleSource #190

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
abangser wants to merge 1 commit into from
+42 −4
Closed

fix: allow empty env var for moduleVersion as can be set in the moduleSource #190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions stages/terraform-module-promise/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ func main() {
yamlFile := GetEnv("KRATIX_INPUT_FILE", "/kratix/input/object.yaml")
outputDir := GetEnv("KRATIX_OUTPUT_DIR", "/kratix/output")
moduleSource := MustHaveEnv("MODULE_SOURCE")
moduleVersion := MustHaveEnv("MODULE_VERSION")
modulePath := os.Getenv("MODULE_PATH") // optional
moduleVersion := os.Getenv("MODULE_VERSION") // optional as can be set with ?ref= in source
modulePath := os.Getenv("MODULE_PATH") // optional

yamlContent, err := os.ReadFile(yamlFile)
if err != nil {
Expand Down Expand Up @@ -44,9 +44,14 @@ func main() {

uniqueFileName := strings.ToLower(fmt.Sprintf("%s_%s_%s", kind, namespace, name))

source := fmt.Sprintf("%s//%s?ref=%s", moduleSource, modulePath, moduleVersion)
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)
}
Comment on lines +47 to 55
Copy link

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.

module := map[string]map[string]map[string]any{
Expand Down
33 changes: 33 additions & 0 deletions stages/terraform-module-promise/test/stage_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,3 +105,36 @@ var _ = Describe("From TF module to Promise Stage", func() {
Expect(string(output)).To(MatchJSON(expectedOutputNoSpec))
})
})

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() {
Copy link

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

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

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.

Suggested change
It("No version provided", func() {
It("No MODULE_VERSION env var provided", func() {

Copilot uses AI. Check for mistakes.
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))
})
})
Comment on lines +109 to +140
Copy link

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

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

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:

  1. MODULE_PATH is set but MODULE_VERSION is empty (to ensure the double-slash doesn't cause issues)
  2. Both MODULE_VERSION is set AND MODULE_SOURCE contains ?ref= (to verify this edge case is handled or at least documented)
  3. 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.

Copilot uses AI. Check for mistakes.