Skip to content

Build-time param defaults in in tkn-bundle#3490

Draft
simonbaird wants to merge 1 commit into
konflux-ci:mainfrom
simonbaird:tkn-bundle-customize-param-defaults
Draft

Build-time param defaults in in tkn-bundle#3490
simonbaird wants to merge 1 commit into
konflux-ci:mainfrom
simonbaird:tkn-bundle-customize-param-defaults

Conversation

@simonbaird
Copy link
Copy Markdown
Member

It's a long story, but we want to reduce the number of moving parts related to updating Conforma in Red Hat Konflux. Being able to pin the policy bundle when building the Conforma tasks means we can reduce breakages related to old incompatible versions of the cli being used with the latest policy bundle.

See also the related PR at conforma/cli#3268

Ref: https://redhat.atlassian.net/browse/EC-1790

@qodo-code-review
Copy link
Copy Markdown

Review Summary by Qodo

Add build-time parameter defaults support to tkn-bundle tasks

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Add PARAM_DEFAULTS parameter to customize task parameter defaults at build time
• Implement parameter default value substitution in tkn-bundle tasks
• Update documentation across multiple README files
• Add comprehensive test coverage for param defaults functionality
Diagram
flowchart LR
  A["PARAM_DEFAULTS input"] -->|"space-separated PARAM=VALUE"| B["Parse entries"]
  B -->|"extract name and value"| C["Update task YAML files"]
  C -->|"yq modify .spec.params"| D["Set default values in bundle"]
  D -->|"push to registry"| E["Bundle with custom defaults"]
Loading

Grey Divider

File Changes

1. task/tkn-bundle/0.2/tkn-bundle.yaml ✨ Enhancement +20/-0

Add PARAM_DEFAULTS parameter and substitution logic

• Add PARAM_DEFAULTS parameter definition with type string and empty default
• Pass PARAM_DEFAULTS environment variable to modify-task-files step
• Implement parameter default value substitution logic using yq
• Parse space-separated PARAM_NAME=VALUE entries and update task YAML files

task/tkn-bundle/0.2/tkn-bundle.yaml


2. task/tkn-bundle-oci-ta/0.2/tkn-bundle-oci-ta.yaml ✨ Enhancement +20/-0

Add PARAM_DEFAULTS parameter and substitution logic

• Add PARAM_DEFAULTS parameter definition with type string and empty default
• Pass PARAM_DEFAULTS environment variable to script step
• Implement identical parameter default value substitution logic using yq
• Parse and apply space-separated PARAM_NAME=VALUE entries to task files

task/tkn-bundle-oci-ta/0.2/tkn-bundle-oci-ta.yaml


3. task/tkn-bundle/0.2/spec/tkn-bundle_spec.sh 🧪 Tests +16/-0

Add test coverage for PARAM_DEFAULTS feature

• Add new test case for PARAM_DEFAULTS functionality
• Verify parameter defaults are correctly set in built bundle
• Validate that custom values override original defaults
• Add bundle inspection command to manual spot-checking documentation

task/tkn-bundle/0.2/spec/tkn-bundle_spec.sh


View more (4)
4. task/tkn-bundle/0.2/spec/test1.yaml 🧪 Tests +4/-0

Add test parameter to test1 task

• Add params section with MY_PARAM parameter definition
• Set original default value to original-value for testing
• Enable test case to verify parameter default override functionality

task/tkn-bundle/0.2/spec/test1.yaml


5. task/tkn-bundle-oci-ta/0.2/README.md 📝 Documentation +1/-0

Document PARAM_DEFAULTS parameter

• Document new PARAM_DEFAULTS parameter in task parameters table
• Specify parameter as optional with empty string default
• Describe functionality for setting default values in task YAML files

task/tkn-bundle-oci-ta/0.2/README.md


6. pipelines/tekton-bundle-builder-oci-ta/README.md 📝 Documentation +1/-0

Document PARAM_DEFAULTS in pipeline README

• Add PARAM_DEFAULTS parameter documentation to pipeline parameters table
• Specify parameter as optional with empty string default
• Describe space-separated PARAM_NAME=VALUE entry format

pipelines/tekton-bundle-builder-oci-ta/README.md


7. pipelines/tekton-bundle-builder/README.md 📝 Documentation +1/-0

Document PARAM_DEFAULTS in pipeline README

• Add PARAM_DEFAULTS parameter documentation to pipeline parameters table
• Specify parameter as optional with empty string default
• Describe space-separated PARAM_NAME=VALUE entry format

pipelines/tekton-bundle-builder/README.md


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented May 1, 2026

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0)

Context used
✅ Tickets: EC-1790

Grey Divider


Action required

1. yq params not null-safe 🐞 Bug ☼ Reliability
Description
The yq expression uses .spec.params[] without a null-safe operator, so YAML files that omit
spec.params can cause yq to exit non-zero and abort the task step under set -o errexit when
PARAM_DEFAULTS is set. This is likely in real bundles because some tasks legitimately have no
params.
Code

task/tkn-bundle/0.2/tkn-bundle.yaml[R193-196]

+          for f in "${FILES[@]}"; do
+            PARAM_NAME="${PARAM_NAME}" PARAM_VALUE="${PARAM_VALUE}" \
+              yq '(.spec.params[] | select(.name == strenv(PARAM_NAME)).default) = strenv(PARAM_VALUE)' -i "$f"
+          done
Evidence
The task applies the yq update across all matched YAML files. The repo itself demonstrates defensive
yq usage with ?. when fields may be absent, and the included test fixtures contain Tasks without
spec.params, which would exercise this behavior when PARAM_DEFAULTS is used.

task/tkn-bundle/0.2/tkn-bundle.yaml[188-198]
task/tkn-bundle/0.2/tkn-bundle.yaml[195-196]
task/tkn-bundle/0.2/spec/test2.yml[1-10]
task/tkn-bundle/0.2/spec/test3.yaml[1-10]
.github/workflows/task-lint.yaml[39-44]

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

### Issue description
The `PARAM_DEFAULTS` implementation updates defaults with:
`yq '(.spec.params[] | select(...).default) = ...' -i`
This assumes `spec.params` exists and is iterable. If a YAML file lacks `spec.params`, yq can fail and the step will exit due to `set -o errexit`.

### Issue Context
The repository already uses null-safe yq patterns (`.spec?.steps[]`) in automation, and test fixtures include Tasks without `spec.params`.

### Fix Focus Areas
- task/tkn-bundle/0.2/tkn-bundle.yaml[188-198]
- task/tkn-bundle-oci-ta/0.2/tkn-bundle-oci-ta.yaml[205-215]

### Suggested fix
Update the yq expression to be null-safe, e.g. one of:
- `(.spec.params[]? | select(.name == strenv(PARAM_NAME)).default) = strenv(PARAM_VALUE)`
- `((.spec.params // [])[] | select(.name == strenv(PARAM_NAME)).default) = strenv(PARAM_VALUE)`

Optionally, add a warning when a file doesn’t contain the named param (similar to the step-name warning logic).

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



Remediation recommended

2. Globbing splits PARAM_DEFAULTS 🐞 Bug ≡ Correctness
Description
The script iterates PARAM_DEFAULTS via an unquoted for entry in ${PARAM_DEFAULTS} while
globstar/nullglob are enabled, so *, ?, and [] in values can expand to filenames and change
which defaults are applied. This can silently set incorrect defaults or drop entries entirely
depending on the current working directory contents.
Code

task/tkn-bundle/0.2/tkn-bundle.yaml[R188-192]

+      if [[ -n "${PARAM_DEFAULTS}" ]]; then
+        for entry in ${PARAM_DEFAULTS}; do
+          PARAM_NAME="${entry%%=*}"
+          PARAM_VALUE="${entry#*=}"
+          echo "Setting param default: ${PARAM_NAME}=${PARAM_VALUE}"
Evidence
The task explicitly enables pathname expansion (globstar/nullglob) and then expands
PARAM_DEFAULTS unquoted in a for loop, which makes the parameter subject to bash word-splitting
and glob expansion. The same pattern exists in the OCI-TA variant, so the behavior is duplicated.

task/tkn-bundle/0.2/tkn-bundle.yaml[83-85]
task/tkn-bundle/0.2/tkn-bundle.yaml[188-196]
task/tkn-bundle-oci-ta/0.2/tkn-bundle-oci-ta.yaml[100-102]
task/tkn-bundle-oci-ta/0.2/tkn-bundle-oci-ta.yaml[205-213]

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

### Issue description
`PARAM_DEFAULTS` is parsed with `for entry in ${PARAM_DEFAULTS}` while globstar/nullglob are enabled. This allows pathname expansion and word-splitting to change entries unexpectedly (e.g., `MY_PARAM=*` expands to filenames).

### Issue Context
This affects both `tkn-bundle` and `tkn-bundle-oci-ta` tasks.

### Fix Focus Areas
- task/tkn-bundle/0.2/tkn-bundle.yaml[83-85]
- task/tkn-bundle/0.2/tkn-bundle.yaml[188-198]
- task/tkn-bundle-oci-ta/0.2/tkn-bundle-oci-ta.yaml[100-102]
- task/tkn-bundle-oci-ta/0.2/tkn-bundle-oci-ta.yaml[205-215]

### Suggested approach
- Disable globbing while parsing: `set -f` / `set +f`.
- Split into an array using `read -r -a` (or `mapfile`) and iterate quoted: `for entry in "${entries[@]}"; do ...`.
- (Optional but recommended) Validate each entry is `NAME=VALUE` and fail fast on invalid entries.

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


Grey Divider

Qodo Logo

Comment thread task/tkn-bundle/0.2/tkn-bundle.yaml
@simonbaird simonbaird force-pushed the tkn-bundle-customize-param-defaults branch from 3dfe5ac to 5631cdb Compare May 1, 2026 15:58
joejstuart
joejstuart previously approved these changes May 1, 2026
@simonbaird simonbaird force-pushed the tkn-bundle-customize-param-defaults branch from 5631cdb to 673ba12 Compare May 1, 2026 18:34
@simonbaird
Copy link
Copy Markdown
Member Author

Last push because I forgot to run hack/generate-ta-tasks.sh.

@simonbaird simonbaird force-pushed the tkn-bundle-customize-param-defaults branch from 673ba12 to 9db4b3e Compare May 1, 2026 18:35
@simonbaird
Copy link
Copy Markdown
Member Author

And that push was just a rebase on fresh upstream/main.

@simonbaird simonbaird force-pushed the tkn-bundle-customize-param-defaults branch from 9db4b3e to 686db14 Compare May 1, 2026 18:51
@simonbaird
Copy link
Copy Markdown
Member Author

Added a version bump and a changelog entry as suggested by the CI.

Copy link
Copy Markdown
Contributor

@chmeliik chmeliik left a comment

Choose a reason for hiding this comment

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

What motivated the approach of modifying the Task YAML in-place at build time instead of pinning the policy bundle in the Task's source code already?

@simonbaird
Copy link
Copy Markdown
Member Author

simonbaird commented May 6, 2026

What motivated the approach of modifying the Task YAML in-place at build time instead of pinning the policy bundle in the Task's source code already?

The high level goal is to be able to update one single artifact (the tekton task bundle for Conforma) and have it specify a pinned and therefore stable Conforma cli and Conforma policy bundle. The cli pinning is done already. The policy pinning is WIP. We're introducing a param called POLICY_BUNDLE_DIGEST in this PR.

Actually in that PR as it is now we are pinning the digest in the source code, but the idea is that we'd prefer to pin it to whatever the current policy bundle is at the time of build (since we'll run some CI to ensure the cli and policy bundle work well together).

This change in the tkn-bundle task means we could pass in the exact digest that was used in the CI, so we get a stable pinned policy bundle, but avoid the chore of needing to regularly update the hard-coded digest in the definition to make sure the policy bundle doesn't get old and stale.

It's a long story, but we want to reduce the number of moving parts
related to updating Conforma in Red Hat Konflux. Being able to pin
the policy bundle when building the Conforma tasks means we can
reduce breakages related to old incompatible versions of the cli
being used with the latest policy bundle.

See also the related PR at conforma/cli#3268

Ref: https://redhat.atlassian.net/browse/EC-1790
Co-authored-by: Claude Code <noreply@anthropic.com>
@simonbaird simonbaird force-pushed the tkn-bundle-customize-param-defaults branch from 686db14 to c783026 Compare May 6, 2026 18:56
@simonbaird
Copy link
Copy Markdown
Member Author

Last revision included some commentary improvements, and a very minor change to the log output. Also I rebased on the latest main branch.

@chmeliik
Copy link
Copy Markdown
Contributor

chmeliik commented May 7, 2026

Actually in that PR as it is now we are pinning the digest in the source code, but the idea is that we'd prefer to pin it to whatever the current policy bundle is at the time of build (since we'll run some CI to ensure the cli and policy bundle work well together).

This change in the tkn-bundle task means we could pass in the exact digest that was used in the CI, so we get a stable pinned policy bundle, but avoid the chore of needing to regularly update the hard-coded digest in the definition to make sure the policy bundle doesn't get old and stale.

What would be the trigger to build and release a new task bundle? Typically, the trigger for Konflux builds is a code change. A Renovate PR that updates the policy bundle seems like the obvious trigger.

How would versioning work? The task will re-release with the same version number but will reference a different version of the policy over time?

@simonbaird
Copy link
Copy Markdown
Member Author

simonbaird commented May 7, 2026

What would be the trigger to build and release a new task bundle? Typically, the trigger for Konflux builds is a code change. A Renovate PR that updates the policy bundle seems like the obvious trigger.

We build a task bundle on every merge. Actually it's done in our Konflux on-push pipeline, see here.

How would versioning work? The task will re-release with the same version number but will reference a different version of the policy over time?

It's a good question. Currently all the tasks are "0.1". We actually don't bump the version. Perhaps we'll change that, but I see it as out of scope for EC-1790.

@chmeliik
Copy link
Copy Markdown
Contributor

chmeliik commented May 7, 2026

What would be the trigger to build and release a new task bundle? Typically, the trigger for Konflux builds is a code change. A Renovate PR that updates the policy bundle seems like the obvious trigger.

We build a task bundle on every merge. Actually it's done in our Konflux on-push pipeline, see here.

How would versioning work? The task will re-release with the same version number but will reference a different version of the policy over time?

It's a good question. Currently all the tasks are "0.1". We actually don't bump the version. Perhaps we'll change that, but I see it as out of scope for EC-1790.

Ack, I see now how it makes sense for your particular setup.

Though modifying the task definition dynamically at build time still doesn't feel like something that should become an accepted pattern. Would you be able to achieve what you need using the run-script task? (https://konflux-ci.dev/docs/patterns/running-user-scripts-on-the-build-pipeline/)

I imagine you will need to execute some custom code in the pipeline to find the bundle digest anyway, so would it work to just edit the YAML in-place using custom code as well?

Copy link
Copy Markdown
Contributor

@chmeliik chmeliik left a comment

Choose a reason for hiding this comment

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

Although, looking closer, STEPS_IMAGE has already set the precedent for build-time edits. Seems reasonable in that context

@simonbaird
Copy link
Copy Markdown
Member Author

Although, looking closer, STEPS_IMAGE has already set the precedent for build-time edits. Seems reasonable in that context

Yeah, the motivation is quite similar actually. For STEP_IMAGE the goal was to use a fresh and pinned task runner image conveniently without needing to hard code the digest anywhere.

The PARAM_DEFAULTS is similar in that we want to use it to pin a policy bundle without needing to hard code its digest, (and hence require some manual update toil, or some complicated automation to update it when necessary).

FWIW, one argument against merging this is that we might be the only team that makes use of it, however I think there may be other use cases for setting a param default at build time.

@simonbaird
Copy link
Copy Markdown
Member Author

We realized that the release service pipelines are using git resolvers, so adjusting the value in the tekton bundle doesn't make any difference for them. A hard coded digest in git is the only thing that does works for them. Until we can move release service pipelines to bundle resolvers, let's put this on hold.

@simonbaird simonbaird marked this pull request as draft May 11, 2026 17:19
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