Skip to content

prefetch: allow outputting multiple env files#87

Merged
chmeliik merged 3 commits intokonflux-ci:mainfrom
chmeliik:prefetch-env-json
Mar 26, 2026
Merged

prefetch: allow outputting multiple env files#87
chmeliik merged 3 commits intokonflux-ci:mainfrom
chmeliik:prefetch-env-json

Conversation

@chmeliik
Copy link
Copy Markdown
Contributor

  1. Fix a problem that caused integration tests to test the kbc version installed in the upstream hermeto image instead of testing the one we want to test
  2. Make it possible to generate multiple env files and to generate JSON env files

The plan is to immediately follow up with a PR that makes the Konflux prefetch task return cachi2.env, prefetch.env and prefetch-env.json (currently only outputs cachi2.env).

Prefetch support in the build task will be implemented as follows:

  • If <prefetch-dir>/prefetch-env.json exists and buildah version >= (not yet released), inject env vars without modifying the containerfile (described more in the commit message)
  • Else if <prefetch-dir>/prefetch.env or <prefetch-dir>/cachi2.env exists, modify the containerfile to source it in each RUN instruction (cachi2.env fallback needed in case the build task is used with an older version of the prefetch task)
  • Possibly always mount the .env file at /cachi2/cachi2.env. It is possible that some users use the existence of the file to decide if they should do some prefetch-specific things. To be investigated.

@chmeliik chmeliik requested a review from a team as a code owner March 24, 2026 12:36
@qodo-code-review
Copy link
Copy Markdown

qodo-code-review bot commented Mar 24, 2026

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 📐 Spec deviations (0)

Grey Divider


Action required

1. GenerateEnv unit test broken🐞 Bug ✓ Correctness
Description
HermetoCli.GenerateEnv now appends --format after --output, but
TestHermetoCliGenerateEnvArgs asserts the old positional argument order, causing unit tests to
fail.
Code

pkg/cliwrappers/hermeto.go[R107-109]

+	if params.Format != "" {
+		args = append(args, "--format", params.Format)
+	}
Evidence
The implementation builds args with --output before conditionally appending --format. The unit
test still expects --format to appear before --output at fixed indices, so the assertion will
fail deterministically.

pkg/cliwrappers/hermeto.go[93-114]
pkg/cliwrappers/hermeto_test.go[81-115]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`pkg/cliwrappers/hermeto.go` changed the ordering of `hermeto generate-env` arguments such that `--format` is appended after `--output`. `pkg/cliwrappers/hermeto_test.go` asserts a specific argument order and now fails.
### Issue Context
- The test currently checks fixed indices (e.g., `capturedArgs[6] == "--format"`), which no longer matches the constructed args.
### Fix Focus Areas
- Either restore the previous argument ordering in `HermetoCli.GenerateEnv` (place `--format` before `--output` when provided), or make the unit test order-insensitive (e.g., assert the presence of flag/value pairs rather than exact positions).
- pkg/cliwrappers/hermeto.go[93-114]
- pkg/cliwrappers/hermeto_test.go[81-115]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Bare env-file silently ignored 🐞 Bug ✓ Correctness
Description
With env-file now a slice flag, a user invoking --env-file without any values can be silently
dropped by ExpandArrayParameters, causing the CLI to fall back to the default env file instead of
erroring.
Code

pkg/commands/prefetch_dependencies/params.go[R66-72]

"env-file": {
Name:         "env-file",
-		TypeKind:     reflect.String,
+		TypeKind:     reflect.Slice,
EnvVarName:   "KBC_PD_ENV_FILE",
DefaultValue: "./prefetch.env",
-		Usage:        "path to file where environment variables for hermetic build will be written",
+		Usage:        "path(s) to file(s) where environment variables for hermetic build will be written, format is inferred from file suffix",
Required:     false,
Evidence
This PR changes env-file from a string to a slice, which makes it subject to
ExpandArrayParameters’ multi-flag expansion. That function currently removes a multi-flag entirely
if no values follow it (it only appends flag,value pairs inside the loop), so a bare --env-file
is not passed to cobra/pflag and no missing-arg error can occur; since the param is optional,
ParseParameters then falls back to the default value.

pkg/commands/prefetch_dependencies/params.go[66-112]
pkg/common/array_arg.go[35-110]
cmd/root.go[19-29]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
After changing `--env-file` to a slice flag, the global `ExpandArrayParameters` preprocessor can now silently drop a bare `--env-file` (no values). This causes the command to proceed using defaults rather than surfacing an argument error.
### Issue Context
- `ExpandArrayParameters` expands `--flag v1 v2` into `--flag v1 --flag v2`.
- For multi-flags, if no non-flag tokens follow, it currently appends nothing (thus removing the flag entirely).
### Fix Focus Areas
- In `ExpandArrayParameters`, when encountering a multi-flag (e.g., `--env-file`) and the next token is missing / `--` / another flag, append the flag as-is so cobra/pflag can report a missing value error.
- Consider adding a small unit test for this behavior if none exists.
- pkg/common/array_arg.go[63-110]
- pkg/commands/prefetch_dependencies/params.go[66-72]
- cmd/root.go[19-29]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

@snyk-io
Copy link
Copy Markdown

snyk-io bot commented Mar 24, 2026

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@chmeliik chmeliik force-pushed the prefetch-env-json branch from 96971e6 to 9498604 Compare March 24, 2026 13:05
@chmeliik
Copy link
Copy Markdown
Contributor Author

@konflux-ci/prefetch @slimreaper35 PTAL if the plan makes sense

Comment on lines +67 to +68
Name: "env-file",
TypeKind: reflect.String,
TypeKind: reflect.Slice,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nitpick: Maybe the name could be --env-files / KBC_PD_ENV_FILES now, but that would require a change in the environment variable in the task.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I wanted to avoid breaking changes, but it's probably OK to do them at this point. I'm fairly sure e2e tests will catch this when build-definitions gets the hermeto image update PR

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Renamed

The integration tests for the prefetch command run in
quay.io/konflux-ci/hermeto:latest and mount the compiled kbc binary to
/usr/bin/konflux-build-cli. But the hermeto image already installs kbc
to /usr/local/bin/konflux-build-cli. This path takes precedence over
/usr/bin, causing the tests to run against the preinstalled binary
rather than the one we actually want to test.

Fix by executing the kbc binary by its full path instead of executing it
only by name.

Signed-off-by: Adam Cmiel <acmiel@redhat.com>
In a not-yet-released (as of making this commit) buildah version, we
will get the ability to set build-time environment variables without
having to modify the Containerfile.

This will be done via a combination of --secret and --mount:

    --secret id=PIP_FIND_LINKS,src=workdir/prefetch-env/PIP_FIND_LINKS

    --mount type=secret,id=PIP_FIND_LINKS,env=PIP_FIND_LINKS

This solution won't be practical with just a prefetch.env file. To find
the names and values of variables to set, the 'image build' command
would have to parse an env file, which can contain shell-quoted strings.

Make it possible to output both a prefetch.env and a prefetch-env.json
(or any other arbitrary file names) at the same time. Hermeto will
select the correct format based on the file extension.

Signed-off-by: Adam Cmiel <acmiel@redhat.com>
For consistency with other kbc flags that accept multiple values, use
plural. Note that this is a breaking change, but since we currently only
have one user (the prefetch-dependencies Tekton task), that should be OK

Signed-off-by: Adam Cmiel <acmiel@redhat.com>
@chmeliik chmeliik force-pushed the prefetch-env-json branch from 9498604 to 35f585f Compare March 24, 2026 14:39
@chmeliik
Copy link
Copy Markdown
Contributor Author

/retest

@chmeliik chmeliik merged commit 1dfad87 into konflux-ci:main Mar 26, 2026
7 checks passed
@chmeliik chmeliik deleted the prefetch-env-json branch March 26, 2026 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants