Add new collect keyless params task#3186
Merged
simonbaird merged 4 commits intoconforma:mainfrom Mar 24, 2026
Merged
Conversation
📝 WalkthroughWalkthroughAdds a new Tekton Task Changes
Sequence Diagram(s)sequenceDiagram
participant Pipeline as Pipeline/Bundle
participant TektonTask as Tekton Task\n(collect-keyless-params)
participant Container as Task Container\n(quay.io/conforma/cli:latest)
participant K8sAPI as Kubernetes API\n(ConfigMap)
Pipeline->>TektonTask: invoke task with params
TektonTask->>Container: start step (bash script)
Container->>K8sAPI: kubectl get configmap\n-o json --ignore-not-found
K8sAPI-->>Container: JSON object or empty/not-found
Container->>Container: parse with jq,\ncompute results (internal→external URL fallback)
Container-->>TektonTask: write Tekton result files
TektonTask-->>Pipeline: publish results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Review Summary by QodoAdd collect-keyless-params task with ConfigMap support and acceptance tests
WalkthroughsDescription• Add new collect-keyless-params Tekton task for reading keyless signing configuration from ConfigMaps • Implement ConfigMap and namespace creation support in acceptance test framework with RBAC permissions • Add comprehensive acceptance test scenarios covering multiple ConfigMap states and edge cases • Include new task in bundle pushes for both Konflux and deprecated GitHub repositories Diagramflowchart LR
A["New Task: collect-keyless-params"] -->|reads config from| B["ConfigMap in konflux-info namespace"]
B -->|extracts parameters| C["Keyless signing configuration"]
D["Test Framework Enhancements"] -->|supports| E["ConfigMap creation"]
D -->|supports| F["Namespace creation"]
D -->|manages| G["RBAC permissions"]
H["Bundle Workflows"] -->|include| A
File Changes1. tasks/collect-keyless-params/0.1/collect-keyless-params.yaml
|
Contributor
Code Review by Qodo
1. Broken docs xref
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
features/task_validate_image.feature (1)
495-517: Add missingbuildIdentityassertion in the “ConfigMap does not exist” scenario.Line [516]-[517] validates
buildIdentityRegexpbut this scenario does not assertbuildIdentity, leaving one result unchecked in this failure path.As per coding guidelines, "-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."Proposed test assertion patch
And the task result "tufExternalUrl" should equal "" + And the task result "buildIdentity" should equal "" And the task result "buildIdentityRegexp" should equal "" And the task result "keylessSigningEnabled" should equal "false"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/task_validate_image.feature` around lines 495 - 517, The "ConfigMap does not exist" scenario for the task "collect-keyless-params" is missing an assertion for the task result "buildIdentity", leaving that output unchecked; add a new step asserting That the task result "buildIdentity" should equal "" (empty string) alongside the existing assertions (e.g., after the "buildIdentityRegexp" and before "keylessSigningEnabled") so the failure path verifies all keyless signing fields produced by the "collect-signing-params" step.acceptance/kubernetes/kind/kubernetes.go (2)
317-320: Same issue: useapierrors.IsAlreadyExistsinstead of string matching.♻️ Proposed fix
// Ignore error if namespace already exists - if err != nil && strings.Contains(err.Error(), "already exists") { + if err != nil && apierrors.IsAlreadyExists(err) { return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acceptance/kubernetes/kind/kubernetes.go` around lines 317 - 320, Replace the string-match check for an existing namespace with the Kubernetes API helper: use apierrors.IsAlreadyExists(err) instead of strings.Contains(err.Error(), "already exists") in the function that handles namespace creation (the block currently checking err != nil && strings.Contains(...)); also add the import for "k8s.io/apimachinery/pkg/api/errors" aliased as apierrors if not already present so the check becomes apierrors.IsAlreadyExists(err) and returns nil when true.
273-278: Useapierrors.IsAlreadyExistsconsistently for error checking.Lines 275 and 301 use string matching on the error message, while line 231 correctly uses
apierrors.IsAlreadyExists(err). The string-based approach is fragile and inconsistent with the rest of this file.♻️ Proposed fix
if _, err := k.client.RbacV1().ClusterRoles().Create(ctx, clusterRole, metav1.CreateOptions{}); err != nil { // Ignore error if ClusterRole already exists - if !strings.Contains(err.Error(), "already exists") { + if !apierrors.IsAlreadyExists(err) { return fmt.Errorf("failed to create ClusterRole: %w", err) } }if _, err := k.client.RbacV1().ClusterRoleBindings().Create(ctx, clusterRoleBinding, metav1.CreateOptions{}); err != nil { // Ignore error if ClusterRoleBinding already exists - if !strings.Contains(err.Error(), "already exists") { + if !apierrors.IsAlreadyExists(err) { return fmt.Errorf("failed to create ClusterRoleBinding: %w", err) } }Also applies to: 299-304
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acceptance/kubernetes/kind/kubernetes.go` around lines 273 - 278, Replace fragile string matching on err.Error() with Kubernetes API error helper: call apierrors.IsAlreadyExists(err) to detect already-existing resources; specifically update the error checks after k.client.RbacV1().ClusterRoles().Create (and the similar check after k.client.RbacV1().ClusterRoleBindings().Create) to use apierrors.IsAlreadyExists(err) and only return an error when that helper returns false, ensuring import of "k8s.io/apimachinery/pkg/api/errors" (alias apierrors) is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@acceptance/kubernetes/kind/kubernetes.go`:
- Around line 317-320: Replace the string-match check for an existing namespace
with the Kubernetes API helper: use apierrors.IsAlreadyExists(err) instead of
strings.Contains(err.Error(), "already exists") in the function that handles
namespace creation (the block currently checking err != nil &&
strings.Contains(...)); also add the import for
"k8s.io/apimachinery/pkg/api/errors" aliased as apierrors if not already present
so the check becomes apierrors.IsAlreadyExists(err) and returns nil when true.
- Around line 273-278: Replace fragile string matching on err.Error() with
Kubernetes API error helper: call apierrors.IsAlreadyExists(err) to detect
already-existing resources; specifically update the error checks after
k.client.RbacV1().ClusterRoles().Create (and the similar check after
k.client.RbacV1().ClusterRoleBindings().Create) to use
apierrors.IsAlreadyExists(err) and only return an error when that helper returns
false, ensuring import of "k8s.io/apimachinery/pkg/api/errors" (alias apierrors)
is present.
In `@features/task_validate_image.feature`:
- Around line 495-517: The "ConfigMap does not exist" scenario for the task
"collect-keyless-params" is missing an assertion for the task result
"buildIdentity", leaving that output unchecked; add a new step asserting That
the task result "buildIdentity" should equal "" (empty string) alongside the
existing assertions (e.g., after the "buildIdentityRegexp" and before
"keylessSigningEnabled") so the failure path verifies all keyless signing fields
produced by the "collect-signing-params" step.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d989645f-b73b-44ac-bade-1a27a46a9f83
⛔ Files ignored due to path filters (1)
features/__snapshots__/task_validate_image.snapis excluded by!**/*.snap
📒 Files selected for processing (10)
.github/workflows/release.yaml.tekton/cli-main-pull-request.yaml.tekton/cli-main-push.yamlacceptance/kubernetes/kind/kubernetes.goacceptance/kubernetes/kubernetes.goacceptance/kubernetes/stub/stub.goacceptance/kubernetes/types/types.godocs/modules/ROOT/partials/tasks_nav.adocfeatures/task_validate_image.featuretasks/collect-keyless-params/0.1/collect-keyless-params.yaml
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
simonbaird
force-pushed
the
keyless-params-task-redo
branch
from
e05fc8f to
19b48d2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tasks/collect-keyless-params/0.1/collect-keyless-params.yaml (1)
142-146: Minor: Log message could be more precise.The message states "enableKeylessSigning is not set" but the condition triggers when the value is anything other than
"true"(including explicit"false"). Consider clarifying:✏️ Suggested improvement
else # Otherwise we ignore the rest of the ConfigMap - echo "enableKeylessSigning is not set, using default empty values" + echo "enableKeylessSigning is not 'true', using default empty values" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tasks/collect-keyless-params/0.1/collect-keyless-params.yaml` around lines 142 - 146, The log is misleading because the else branch runs for any value other than "true"; update the echo in the else branch that mentions enableKeylessSigning so it clearly states the value isn't "true" (and ideally print the actual variable), e.g. change the message currently printed by the else branch to indicate enableKeylessSigning != "true" and include the variable value (e.g., enableKeylessSigning="$enableKeylessSigning") while keeping the note about using default empty values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tasks/collect-keyless-params/0.1/collect-keyless-params.yaml`:
- Around line 142-146: The log is misleading because the else branch runs for
any value other than "true"; update the echo in the else branch that mentions
enableKeylessSigning so it clearly states the value isn't "true" (and ideally
print the actual variable), e.g. change the message currently printed by the
else branch to indicate enableKeylessSigning != "true" and include the variable
value (e.g., enableKeylessSigning="$enableKeylessSigning") while keeping the
note about using default empty values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: caacea4b-f132-4178-821d-44b0d74c7866
⛔ Files ignored due to path filters (1)
features/__snapshots__/task_validate_image.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
.github/workflows/release.yaml.tekton/cli-main-pull-request.yaml.tekton/cli-main-push.yamlacceptance/kubernetes/kind/kubernetes.goacceptance/kubernetes/kubernetes.goacceptance/kubernetes/stub/stub.goacceptance/kubernetes/types/types.godocs/modules/ROOT/pages/collect-keyless-params.adocdocs/modules/ROOT/partials/tasks_nav.adocfeatures/task_validate_image.featuretasks/collect-keyless-params/0.1/collect-keyless-params.yaml
✅ Files skipped from review due to trivial changes (3)
- docs/modules/ROOT/partials/tasks_nav.adoc
- docs/modules/ROOT/pages/collect-keyless-params.adoc
- acceptance/kubernetes/kind/kubernetes.go
🚧 Files skipped from review as they are similar to previous changes (5)
- acceptance/kubernetes/types/types.go
- .tekton/cli-main-pull-request.yaml
- acceptance/kubernetes/stub/stub.go
- .github/workflows/release.yaml
- features/task_validate_image.feature
|
@simonbaird for getting the identity of chains, please use the |
|
@simonbaird the default should be to use the internal Sigstore urls https://github.com/redhat-appstudio/tsf-cli/blob/84561ca6c9a39272a671c2ba5aeafef648dfe566/installer/charts/tsf-konflux/templates/konflux.yaml#L61 If those are not present in the configmap (which means sigstore runs out of the cluster), the external urls should be used. |
(A test will be added in some upcoming commits.) Ref: https://redhat.atlassian.net/browse/EC-1695 Co-authored-by: Claude Code <noreply@anthropic.com>
Also namespaces, since we want the ConfigMap in a particular namespace. An RBAC is created also so the ConfigMap is readable by every service account. This will be used in the acceptance test added in an upcoming commit. Ref: https://issues.redhat.com/browse/EC-1695 Co-authored-by: Claude Code <noreply@anthropic.com>
simonbaird
force-pushed
the
keyless-params-task-redo
branch
from
19b48d2 to
baf6e7d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
acceptance/kubernetes/kind/kubernetes.go (2)
273-278: Useapierrors.IsAlreadyExistsconsistently.String matching on error messages is fragile. Since
apierrorsis already imported and used correctly inCreateConfigMap(line 231), use the same approach here.♻️ Proposed fix
if _, err := k.client.RbacV1().ClusterRoles().Create(ctx, clusterRole, metav1.CreateOptions{}); err != nil { - // Ignore error if ClusterRole already exists - if !strings.Contains(err.Error(), "already exists") { + if !apierrors.IsAlreadyExists(err) { return fmt.Errorf("failed to create ClusterRole: %w", err) } }if _, err := k.client.RbacV1().ClusterRoleBindings().Create(ctx, clusterRoleBinding, metav1.CreateOptions{}); err != nil { - // Ignore error if ClusterRoleBinding already exists - if !strings.Contains(err.Error(), "already exists") { + if !apierrors.IsAlreadyExists(err) { return fmt.Errorf("failed to create ClusterRoleBinding: %w", err) } }Also applies to: 299-304
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acceptance/kubernetes/kind/kubernetes.go` around lines 273 - 278, Replace fragile string matching with apierrors.IsAlreadyExists when handling create errors for ClusterRole creation: in the block that calls k.client.RbacV1().ClusterRoles().Create(ctx, clusterRole, metav1.CreateOptions{}), catch the error and use apierrors.IsAlreadyExists(err) to ignore "already exists" cases and return other errors (same pattern used in CreateConfigMap). Apply the same change to the other create block noted (the ClusterRoleBinding/related create at the later section) so both use apierrors.IsAlreadyExists instead of strings.Contains(err.Error(), "already exists").
317-320: Same issue: useapierrors.IsAlreadyExistshere as well.♻️ Proposed fix
- // Ignore error if namespace already exists - if err != nil && strings.Contains(err.Error(), "already exists") { + if apierrors.IsAlreadyExists(err) { return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acceptance/kubernetes/kind/kubernetes.go` around lines 317 - 320, Replace the string-based check for an existing namespace with the k8s API helper: instead of checking strings.Contains(err.Error(), "already exists") in the namespace-creation error handling (the block that currently does "if err != nil && strings.Contains(err.Error(), \"already exists\") { return nil }"), use apierrors.IsAlreadyExists(err) to detect that condition; update imports to include "k8s.io/apimachinery/pkg/api/errors" aliased as apierrors and keep the logic to return nil when apierrors.IsAlreadyExists(err).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@acceptance/kubernetes/kind/kubernetes.go`:
- Around line 273-278: Replace fragile string matching with
apierrors.IsAlreadyExists when handling create errors for ClusterRole creation:
in the block that calls k.client.RbacV1().ClusterRoles().Create(ctx,
clusterRole, metav1.CreateOptions{}), catch the error and use
apierrors.IsAlreadyExists(err) to ignore "already exists" cases and return other
errors (same pattern used in CreateConfigMap). Apply the same change to the
other create block noted (the ClusterRoleBinding/related create at the later
section) so both use apierrors.IsAlreadyExists instead of
strings.Contains(err.Error(), "already exists").
- Around line 317-320: Replace the string-based check for an existing namespace
with the k8s API helper: instead of checking strings.Contains(err.Error(),
"already exists") in the namespace-creation error handling (the block that
currently does "if err != nil && strings.Contains(err.Error(), \"already
exists\") { return nil }"), use apierrors.IsAlreadyExists(err) to detect that
condition; update imports to include "k8s.io/apimachinery/pkg/api/errors"
aliased as apierrors and keep the logic to return nil when
apierrors.IsAlreadyExists(err).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 99e5328b-8c7e-41f8-8168-81187e26252f
⛔ Files ignored due to path filters (1)
features/__snapshots__/task_validate_image.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
.github/workflows/release.yaml.tekton/cli-main-pull-request.yaml.tekton/cli-main-push.yamlacceptance/kubernetes/kind/kubernetes.goacceptance/kubernetes/kubernetes.goacceptance/kubernetes/stub/stub.goacceptance/kubernetes/types/types.godocs/modules/ROOT/pages/collect-keyless-params.adocdocs/modules/ROOT/partials/tasks_nav.adocfeatures/task_validate_image.featuretasks/collect-keyless-params/0.1/collect-keyless-params.yaml
✅ Files skipped from review due to trivial changes (3)
- docs/modules/ROOT/partials/tasks_nav.adoc
- acceptance/kubernetes/types/types.go
- docs/modules/ROOT/pages/collect-keyless-params.adoc
🚧 Files skipped from review as they are similar to previous changes (6)
- .tekton/cli-main-pull-request.yaml
- acceptance/kubernetes/stub/stub.go
- .tekton/cli-main-push.yaml
- .github/workflows/release.yaml
- tasks/collect-keyless-params/0.1/collect-keyless-params.yaml
- features/task_validate_image.feature
Member
Author
|
@gbenhaim Thanks for the review. New revision adds the Also I adjusted the logic so we use the internal urls if they exist, and fall back to the external urls if there are no internal urls. There's a related change to the three url task results, e.g. I'm using |
|
/review |
Contributor
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
gbenhaim
reviewed
Mar 24, 2026
gbenhaim
approved these changes
Mar 24, 2026
st3penta
previously approved these changes
Mar 24, 2026
Contributor
st3penta
left a comment
st3penta
left a comment
There was a problem hiding this comment.
left a few minor comments, but overall LGTM, feel free to merge it
There are pushes happening in both Konflux and GitHub, and each of those pushes to the new and old locations. I added the new task everywhere, even the deprecated repo. Ref: https://redhat.atlassian.net/browse/EC-1695
simonbaird
force-pushed
the
keyless-params-task-redo
branch
from
baf6e7d to
ea67e32
Compare
Member
Author
|
Added some extra comments in the acceptance test, and changed cluster-config-0 to -1. |
Member
Author
|
I'll merge once CI is green. |
joejstuart
approved these changes
Mar 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The task will look in a ConfigMap containing Konflux cluster config related to keyless signing. The idea is to use the task results as input params to the Conforma task so it can perform keyless signature verification using the correct params.
Ref: https://redhat.atlassian.net/browse/EC-1695
Note: This was split out from #3171 which has been blocked on some unrelated acceptance test problems because I was doing too much in the one PR. This PR has the task only, so should be green and ready to merge.