Add new collect keyless params task#3186
Open
simonbaird wants to merge 4 commits intoconforma:mainfrom
Open
Conversation
📝 WalkthroughWalkthroughAdds a new Tekton Task Changes
Sequence Diagram(s)sequenceDiagram
participant Pipeline as Pipeline/Bundle
participant TektonTask as Tekton Task (collect-keyless-params)
participant Container as Task Container (cli:latest)
participant K8sAPI as Kubernetes API (ConfigMap)
Pipeline->>TektonTask: invoke task with params
TektonTask->>Container: start step (bash script)
Container->>K8sAPI: kubectl get configmap -o json
K8sAPI-->>Container: JSON (or empty / not found)
Container->>Container: parse with jq, compute results
Container-->>TektonTask: write tekton results
TektonTask-->>Pipeline: publish results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.3)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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:
|
(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>
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
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
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
1 participant
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 container 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.