Skip to content

fix(api): replace runner list overrides - #951

Open
floze-the-genius wants to merge 2 commits into
padok-team:mainfrom
floze-the-genius:fix/420-replace-runner-lists
Open

fix(api): replace runner list overrides#951
floze-the-genius wants to merge 2 commits into
padok-team:mainfrom
floze-the-genius:fix/420-replace-runner-lists

Conversation

@floze-the-genius

Copy link
Copy Markdown

Summary

  • make all remaining OverrideRunnerSpec list fields use layer-level replacement instead of merging repository and layer values
  • preserve repository lists when the layer does not provide an override
  • strengthen regression tests with full ordered structure comparisons while treating nil and empty collections equivalently

Closes #420

Validation

  • go test ./api/v1alpha1
  • go vet ./...
  • make test (Kubernetes envtest 1.36.0 plus Azurite, MinIO, LocalStack, and fake GCS Docker services; cleanup completed)
  • git diff --check

AI assistance disclosure

I used OpenAI Codex to help analyze the issue, implement the change, and update the regression tests. I reviewed the resulting diff and ran the validation commands listed above.

@github-project-automation github-project-automation Bot moved this to 📋 Backlog in Burrito Jul 18, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4be731229b

ℹ️ 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".

Comment thread api/v1alpha1/common.go Outdated
@floze-the-genius

Copy link
Copy Markdown
Author

Quick review follow-up for the library maintainers: the sole automated review thread was fixed in bd7529c and is resolved, and GitHub still reports the PR mergeable. The remaining gates appear to be code-owner approval and the required commitlint/CI runs; this fork currently has no workflow runs, so it may need maintainer approval to start them. Could someone take a look when convenient? I can rebase the 12 upstream commits if preferred.

@DjinnS

DjinnS commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

⚠️ The semantic change to ChooseSlice (api/v1alpha1/common.go:179, len(b) > 0b != nil) is correct for the repository/layer precedence case, but this function is also reused in internal/controllers/terraformrun/pod.go:167-168 to combine the default pod spec (config.Runner.Command / config.Runner.Args, e.g. ["burrito"] / ["runner", "start"]) with the result of GetOverrideRunnerSpec:

defaultSpec.Containers[0].Args = configv1alpha1.ChooseSlice(defaultSpec.Containers[0].Args, overrideSpec.Args)
defaultSpec.Containers[0].Command = configv1alpha1.ChooseSlice(defaultSpec.Containers[0].Command, overrideSpec.Command)

With the new semantics, if a user sets overrideRunnerSpec.args: [] (or command: []) explicitly on a TerraformRepository/TerraformLayer — e.g. from a templated manifest that always emits [] instead of omitting the field — overrideSpec.Args becomes a non-nil empty slice, and this line then overwrites the runner's default Command/Args with []. The runner pod starts with no command, silently breaking every plan/apply for that layer.

Before this PR, that same explicit empty override was a no-op (it fell back to the defaults) — this is a regression caused by sharing ChooseSlice between two different needs:

  1. repository vs layer precedence (where distinguishing nil vs empty is exactly what this PR wants);
  2. hardcoded defaults vs user override (where an empty override should remain a no-op, not clear the default).

Suggested fix: keep the public ChooseSlice semantics unchanged (len(b) > 0, as still used in pod.go), and introduce a dedicated function for the repository/layer case, e.g.:

// used only for repository/layer OverrideRunnerSpec precedence,
// where an explicit empty slice must clear the repository's value.
func overrideSlice(a, b []string) []string {
    if b != nil {
        return b
    }
    return a
}

and use it in GetOverrideRunnerSpec for Command/Args instead of ChooseSlice. That isolates the intended behavior change (#420) without affecting the defaults/override composition in pod.go. A test in internal/controllers/terraformrun/pod_test.go covering overrideRunnerSpec.args: [] would also help lock this case down.

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

Projects

Status: 📋 Backlog

Development

Successfully merging this pull request may close these issues.

Apply the new merge behavior for list everywhere and fix the OverrideRunnerSpec test

3 participants