Red Hat Konflux kflux-prd-rh03 update task-build-npm-package - #303
Open
red-hat-konflux-kflux-prd-rh03[bot] wants to merge 1 commit into
Open
Red Hat Konflux kflux-prd-rh03 update task-build-npm-package#303red-hat-konflux-kflux-prd-rh03[bot] wants to merge 1 commit into
red-hat-konflux-kflux-prd-rh03[bot] wants to merge 1 commit into
Conversation
Signed-off-by: red-hat-konflux-kflux-prd-rh03 <konflux@no-reply.konflux-ci.dev>
Reviewer's GuideAdds two Konflux Pipelines-as-Code Tekton PipelineRun definitions for the task-build-npm-package component, one triggered on pull requests to main and one on pushes to main, wiring together standard Konflux catalog tasks to clone the repo, prefetch dependencies, build and index a container image, run SAST checks, and apply tags, with appropriate metadata, parameters, and workspaces. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The pull-request and push PipelineRun YAMLs are almost identical; consider factoring out the common
pipelineSpecinto a reusable template or referencing a shared Pipeline to reduce duplication and the risk of the two diverging unintentionally. - Several params defined in
pipelineSpec(e.g.,dockerfile,hermetic,build-source-image) are not actually passed into any tasks; either wire these through to the relevant tasks or remove them to avoid confusion about their effect.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The pull-request and push PipelineRun YAMLs are almost identical; consider factoring out the common `pipelineSpec` into a reusable template or referencing a shared Pipeline to reduce duplication and the risk of the two diverging unintentionally.
- Several params defined in `pipelineSpec` (e.g., `dockerfile`, `hermetic`, `build-source-image`) are not actually passed into any tasks; either wire these through to the relevant tasks or remove them to avoid confusion about their effect.
## Individual Comments
### Comment 1
<location path=".tekton/task-build-npm-package-pull-request.yaml" line_range="53-62" />
<code_context>
+ 1h, 2d, 3w for hours, days, and weeks, respectively.
+ name: image-expires-after
+ type: string
+ - default: "false"
+ description: Build a source image.
+ name: build-source-image
+ type: string
+ - default: "false"
+ description: Add built image into an OCI image index
+ name: build-image-index
+ type: string
+ - default: "false"
+ description: Enable cache proxy configuration
+ name: enable-cache-proxy
+ - default: "true"
+ description: Use the package registry proxy when prefetching dependencies
</code_context>
<issue_to_address>
**suggestion:** The `enable-cache-proxy` param is missing an explicit `type`, unlike the other params.
`enable-cache-proxy` (and the equivalent in the push manifest) doesn’t declare `type: string` like the other params, and Tekton v1 generally expects an explicit type. Please add `type: string` here (and to `enable-package-registry-proxy` for consistency), unless there’s a deliberate reason to rely on the implicit default.
Suggested implementation:
```
- default: "false"
description: Enable cache proxy configuration
name: enable-cache-proxy
type: string
- default: "true"
description: Use the package registry proxy when prefetching dependencies
name: enable-package-registry-proxy
type: string
```
You mentioned an equivalent parameter in the push manifest. To keep behavior consistent, apply the same `type: string` addition to `enable-cache-proxy` and `enable-package-registry-proxy` (or their equivalents) in the push task YAML (likely something like `.tekton/task-build-npm-package.yaml` or similar) using the same pattern.
</issue_to_address>
### Comment 2
<location path=".tekton/task-build-npm-package-push.yaml" line_range="121-122" />
<code_context>
+ value: $(params.revision)
+ - name: ociStorage
+ value: $(params.output-image).git
+ - name: ociArtifactExpiresAfter
+ value: $(params.image-expires-after)
+ runAfter:
+ - init
</code_context>
<issue_to_address>
**issue (bug_risk):** The push PipelineRun never sets `image-expires-after`, so OCI artifacts may not expire as intended.
`image-expires-after` is declared in `pipelineSpec.params` and used for `ociArtifactExpiresAfter` in `clone-repository` and `prefetch-dependencies`, but the PipelineRun’s top-level `spec.params` doesn’t set it. This leaves it as an empty string, which usually means no expiration. Since the pull-request PipelineRun sets `image-expires-after: 5d`, consider adding a corresponding `image-expires-after` value to the push PipelineRun `spec.params` to ensure push artifacts expire as intended.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+53
to
+62
| - default: "false" | ||
| description: Skip checks against built image | ||
| name: skip-checks | ||
| type: string | ||
| - default: "false" | ||
| description: Execute the build with network isolation | ||
| name: hermetic | ||
| type: string | ||
| - default: "" | ||
| description: Build dependencies to be prefetched |
There was a problem hiding this comment.
suggestion: The enable-cache-proxy param is missing an explicit type, unlike the other params.
enable-cache-proxy (and the equivalent in the push manifest) doesn’t declare type: string like the other params, and Tekton v1 generally expects an explicit type. Please add type: string here (and to enable-package-registry-proxy for consistency), unless there’s a deliberate reason to rely on the implicit default.
Suggested implementation:
- default: "false"
description: Enable cache proxy configuration
name: enable-cache-proxy
type: string
- default: "true"
description: Use the package registry proxy when prefetching dependencies
name: enable-package-registry-proxy
type: string
You mentioned an equivalent parameter in the push manifest. To keep behavior consistent, apply the same type: string addition to enable-cache-proxy and enable-package-registry-proxy (or their equivalents) in the push task YAML (likely something like .tekton/task-build-npm-package.yaml or similar) using the same pattern.
Comment on lines
+121
to
+122
| - name: ociArtifactExpiresAfter | ||
| value: $(params.image-expires-after) |
There was a problem hiding this comment.
issue (bug_risk): The push PipelineRun never sets image-expires-after, so OCI artifacts may not expire as intended.
image-expires-after is declared in pipelineSpec.params and used for ociArtifactExpiresAfter in clone-repository and prefetch-dependencies, but the PipelineRun’s top-level spec.params doesn’t set it. This leaves it as an empty string, which usually means no expiration. Since the pull-request PipelineRun sets image-expires-after: 5d, consider adding a corresponding image-expires-after value to the push PipelineRun spec.params to ensure push artifacts expire as intended.
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
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.
Pipelines as Code configuration proposal
To start the PipelineRun, add a new comment with content
/ok-to-testFor more detailed information about running a PipelineRun, please refer to Pipelines as Code documentation Running the PipelineRun
To customize the proposed PipelineRuns after merge, please refer to Build Pipeline customization
Please follow the block sequence indentation style introduced by the proprosed PipelineRuns YAMLs, or keep using consistent indentation level through your customized PipelineRuns. When different levels are mixed, it will be changed to the proposed style.
Summary by Sourcery
Add Tekton PipelineRun definitions to build and scan the task-build-npm-package image for pull requests and pushes to main.
New Features: