[WIP] ECOPROJECT-3167 | build: Set the UI version from konflux builds #179
Conversation
|
Caution There are some errors in your PipelineRun template.
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughRemoved a branch PipelineRun; added an inline Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Tekton as Tekton Pipeline
participant Clone as clone-repository
participant Update as update-version (inline)
participant Hack as hack/version.sh
participant Prefetch as prefetch-dependencies
participant Build as build steps
Tekton->>Clone: start clone-repository
Clone-->>Update: provide source workspace
Update->>Hack: run --update-package-json (install jq, patch package.json)
Hack-->>Update: patched package.json / exports vars
Update-->>Prefetch: complete -> trigger prefetch-dependencies
Prefetch-->>Build: prefetch done -> build runs (IMAGE_TAG from git)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas to pay attention to:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (9)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.tekton/migration-planner-ui-tag.yaml (1)
193-227: LGTM! Update-version task for tag pipeline.The task implementation is correct, using
{{git_tag}}as the SOURCE_GIT_TAG value, which is appropriate for tag-triggered builds.Note: The code duplication concern mentioned in the pull-request pipeline review also applies here.
.tekton/migration-planner-ui-push.yaml (1)
193-227: LGTM! Update-version task for push pipeline.The task implementation is correct, using
{{target_branch}}-devas the SOURCE_GIT_TAG value, which is appropriate for development builds triggered by pushes to master.Note: The YAMLlint syntax error on line 198 is a false positive. The code duplication concern mentioned earlier also applies here.
🧹 Nitpick comments (1)
.tekton/migration-planner-ui-pull-request.yaml (1)
196-230: Consider extracting update-version as a reusable Tekton Task.The
update-versiontask is duplicated across three pipeline files (.tekton/migration-planner-ui-pull-request.yaml, .tekton/migration-planner-ui-tag.yaml, and .tekton/migration-planner-ui-push.yaml) with nearly identical implementations. This creates maintenance burden.Consider creating a shared Tekton Task definition in a separate file that can be referenced by all three pipelines, reducing duplication and improving maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.tekton/migration-planner-ui-branch-push.yaml(0 hunks).tekton/migration-planner-ui-pull-request.yaml(1 hunks).tekton/migration-planner-ui-push.yaml(1 hunks).tekton/migration-planner-ui-tag.yaml(2 hunks)Makefile(2 hunks)hack/version.sh(1 hunks)package.json(1 hunks)src/common/version.ts(2 hunks)
💤 Files with no reviewable changes (1)
- .tekton/migration-planner-ui-branch-push.yaml
🧰 Additional context used
🪛 ESLint
src/common/version.ts
[error] 1-3: Run autofix to sort these imports!
(simple-import-sort/imports)
🪛 YAMLlint (1.37.1)
.tekton/migration-planner-ui-pull-request.yaml
[error] 201-201: syntax error: expected , but found ''
(syntax)
.tekton/migration-planner-ui-push.yaml
[error] 198-198: syntax error: expected , but found ''
(syntax)
🔇 Additional comments (6)
Makefile (2)
14-14: LGTM! Dynamic IMAGE_TAG computation.The change from static git variables to runtime computation via
git rev-parseis correct and aligns with the centralized versioning approach.
41-41: LGTM! Version script integration.Sourcing
hack/version.shbefore npm build/start commands correctly sets theMIGRATION_PLANNER_UI_GIT_COMMITandMIGRATION_PLANNER_UI_VERSIONenvironment variables at build time.Also applies to: 48-48, 55-55
package.json (1)
3-4: LGTM! Version metadata fields added.The new
uiVersionNameanduiGitCommitfields with "unknown" defaults are appropriate. These will be updated byhack/version.shduring builds and serve as fallbacks insrc/common/version.ts.src/common/version.ts (1)
36-36: LGTM! Fallback to build manifest.The changes to use
buildManifest.uiVersionNameandbuildManifest.uiGitCommitas fallbacks instead of hardcoded 'unknown' strings correctly integrate with the new versioning flow.Also applies to: 44-44
.tekton/migration-planner-ui-pull-request.yaml (1)
196-230: Update-version task implementation is correct.The new task correctly:
- Installs
jqin the UBI minimal image- Exports
SOURCE_GIT_COMMITandSOURCE_GIT_TAGfrom task params- Executes
hack/version.sh --update-package-jsonto update package.json- Runs after
clone-repositoryand beforeprefetch-dependenciesNote: The YAMLlint syntax error on line 201 is a false positive. The
{{target_branch}}syntax is valid PipelinesAsCode template syntax.hack/version.sh (1)
1-59: LGTM! Well-structured version management script.The script demonstrates good practices:
- Proper bash error handling (
set -euo pipefail)- Fallback logic for git commit and tag retrieval
- Environment variable support for CI/CD overrides
- Function separation with execution mode detection
- Error handling for missing dependencies (jq)
The
git describepattern correctly matches semantic versions with or without 'v' prefix.
nirarg
force-pushed
the
version-konflux
branch
from
6b5e46b to
5a3680b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.tekton/migration-planner-ui-push.yaml (1)
193-227: Consider extracting the sharedupdate-versiontask.The
update-versiontask definition is identical across all three pipeline files (pull-request, tag, push). To reduce duplication and maintenance burden, consider:
- Defining this as a reusable Tekton task in a separate
.tekton/tasks/update-version.yaml- Referencing it with
taskRefin all three pipelinesThis would make future updates (e.g., jq version, script path) single-point changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.tekton/migration-planner-ui-branch-push.yaml(0 hunks).tekton/migration-planner-ui-pull-request.yaml(1 hunks).tekton/migration-planner-ui-push.yaml(1 hunks).tekton/migration-planner-ui-tag.yaml(2 hunks)Makefile(2 hunks)hack/version.sh(1 hunks)package.json(1 hunks)src/common/version.ts(2 hunks)
💤 Files with no reviewable changes (1)
- .tekton/migration-planner-ui-branch-push.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- src/common/version.ts
- hack/version.sh
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.tekton/migration-planner-ui-push.yaml
[error] 198-198: syntax error: expected , but found ''
(syntax)
.tekton/migration-planner-ui-pull-request.yaml
[error] 201-201: syntax error: expected , but found ''
(syntax)
🔇 Additional comments (5)
package.json (1)
3-4: LGTM — Version field defaults are appropriate.The addition of
uiVersionNameanduiGitCommitwith "unknown" defaults provides safe fallback values that will be updated by the runtime version script..tekton/migration-planner-ui-tag.yaml (1)
25-25: Update aligns with PR objectives.The change to use
{{revision}}for the image tag is consistent with the shift to runtime versioning driven by Konflux.Makefile (2)
14-14: IMAGE_TAG calculation improved for local dev.Using
git rev-parseinstead of static variables makes the Makefile more robust for local development.
41-41: All verification checks passed.The three targets (build-standalone, run-standalone, and build) correctly and exclusively source hack/version.sh, which properly exports MIGRATION_PLANNER_UI_VERSION and MIGRATION_PLANNER_UI_GIT_COMMIT. The podman-build target intentionally uses IMAGE_TAG instead of the version script, which is an appropriate design choice for containerization. No issues found.
.tekton/migration-planner-ui-pull-request.yaml (1)
213-227: No issues found—all requirements verified.The script exists at
hack/version.shwith:
- Correct variable mapping from
SOURCE_GIT_COMMIT/SOURCE_GIT_TAG(exported by Tekton) toGIT_COMMIT/GIT_TAG(used by script)- Proper exports of
MIGRATION_PLANNER_UI_GIT_COMMITandMIGRATION_PLANNER_UI_VERSION- Working
update_package_json()function that usesjqto updatepackage.json- Proper
--update-package-jsonflag handling- Error handling for missing
jqand git fallbacks
nirarg
force-pushed
the
version-konflux
branch
3 times, most recently
from
3d6e6dd to
a2d2697
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.tekton/migration-planner-ui-tag.yaml (1)
193-249: Apply the same parameter quoting fix as other pipelines.The update-version task here is identical to the pull-request and push versions. Apply the same quoting fix for lines 234-235:
- export SOURCE_GIT_COMMIT=$(params.SOURCE_GIT_COMMIT) - export SOURCE_GIT_TAG=$(params.SOURCE_GIT_TAG) + export SOURCE_GIT_COMMIT="$(params.SOURCE_GIT_COMMIT)" + export SOURCE_GIT_TAG="$(params.SOURCE_GIT_TAG)"
🧹 Nitpick comments (3)
hack/version.sh (1)
20-26: Verify git command robustness for SOURCE_GIT_COMMIT and SOURCE_GIT_TAG fallback logic.Lines 21-22 derive git information with fallbacks. The logic reads environment variables
SOURCE_GIT_COMMITandSOURCE_GIT_TAGfirst (which Tekton task parameters should set), then falls back to local git commands. This is sound, but confirm:
- Does the Tekton task correctly export
SOURCE_GIT_COMMITandSOURCE_GIT_TAGbefore sourcing this script?- Is the git tag pattern matching (
[0-9]*\.[0-9]*\.[0-9]*andv[0-9]*\.[0-9]*\.[0-9]*) sufficient for your versioning scheme, or should other tag patterns be allowed?Consider slightly more robust escaping in the
git describecommand:-GIT_TAG="${SOURCE_GIT_TAG:-$(git describe --tags --always --abbrev=7 --match '[0-9]*\.[0-9]*\.[0-9]*' --match 'v[0-9]*\.[0-9]*\.[0-9]*' 2>/dev/null || echo 'v0.0.0-unknown')}" +GIT_TAG="${SOURCE_GIT_TAG:-$(git describe --tags --always --abbrev=7 --match='[0-9]*\.[0-9]*\.[0-9]*' --match='v[0-9]*\.[0-9]*\.[0-9]*' 2>/dev/null || echo 'v0.0.0-unknown')}"This uses
--match=syntax for consistency and clarity, though both forms are equivalent..tekton/migration-planner-ui-push.yaml (1)
193-249: Consistent structure with pull-request pipeline; apply the same parameter quoting fix.The update-version task here mirrors the pull-request version with appropriate tag naming ("{{target_branch}}-dev"). Apply the same parameter quoting recommendation for lines 234-235:
- export SOURCE_GIT_COMMIT=$(params.SOURCE_GIT_COMMIT) - export SOURCE_GIT_TAG=$(params.SOURCE_GIT_TAG) + export SOURCE_GIT_COMMIT="$(params.SOURCE_GIT_COMMIT)" + export SOURCE_GIT_TAG="$(params.SOURCE_GIT_TAG)"Makefile (1)
38-56: Verify bash sourcing pattern in make targets works as intended.Lines 41, 48, and 55 use the pattern
@bash -c '. ./hack/version.sh && npm run ...'to source the version script before running npm commands. This is sound:
- The dot-source syntax (
. ./hack/version.sh) sources the script in the current shell.- The
&&ensures npm only runs if sourcing succeeds.- Exported variables from the script become available to npm.
- The
set -ein hack/version.sh ensures the script exits on error.However, consider whether you want explicit error handling if version.sh fails. Currently, if sourcing fails, the make target will fail silently (just exit with non-zero). This is likely desired behavior for CI/CD, but you may want to add a comment explaining this.
Consider adding a comment to explain the error handling:
build-standalone: install @echo "Building standalone application..." rm -rf dist-standalone + @# Source version.sh to set MIGRATION_PLANNER_UI_* environment variables; fails fast if git unavailable @bash -c '. ./hack/version.sh && npm run build:standalone'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.tekton/migration-planner-ui-branch-push.yaml(0 hunks).tekton/migration-planner-ui-pull-request.yaml(1 hunks).tekton/migration-planner-ui-push.yaml(1 hunks).tekton/migration-planner-ui-tag.yaml(2 hunks)Makefile(2 hunks)hack/version.sh(1 hunks)package.json(1 hunks)src/common/version.ts(2 hunks)
💤 Files with no reviewable changes (1)
- .tekton/migration-planner-ui-branch-push.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- src/common/version.ts
- package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
hack/version.sh (2)
29-49: Approve the update_package_json function implementation.The function correctly:
- Validates jq is available before attempting updates.
- Uses jq with
--argparameters to safely inject variable values, avoiding injection risks.- Creates a temporary file and moves it atomically to replace the original.
- Provides clear feedback on success.
51-59: Verify the BASH_SOURCE check for script execution vs. sourcing.The conditional at line 52 correctly distinguishes execution from sourcing using
"${BASH_SOURCE[0]}" == "${0}". When executed directly, it handles the--update-package-jsonflag; when sourced, it sets environment variables silently. This design is sound for CI/CD integration..tekton/migration-planner-ui-pull-request.yaml (2)
196-252: Verify the update-version task implementation and shell syntax in the inline script.The new update-version task looks well-structured. A few observations:
Line 237-238: The script uses
$(params.SOURCE_GIT_COMMIT)syntax without quotes:export SOURCE_GIT_COMMIT=$(params.SOURCE_GIT_COMMIT) export SOURCE_GIT_TAG=$(params.SOURCE_GIT_TAG)If these parameters contain special characters or spaces, they should be quoted:
export SOURCE_GIT_COMMIT="$(params.SOURCE_GIT_COMMIT)" export SOURCE_GIT_TAG="$(params.SOURCE_GIT_TAG)"Line 218: Installing jq via
microdnfin the UBI minimal image is appropriate, but consider that this adds a small build-time cost to every pull-request build. This is acceptable for reliability.Workspace and parameter naming: Consistent and clear. The debug output (lines 220-249) is helpful for troubleshooting.
Consider adding quotes around parameter expansions for robustness:
# Run the version script to update package.json - export SOURCE_GIT_COMMIT=$(params.SOURCE_GIT_COMMIT) - export SOURCE_GIT_TAG=$(params.SOURCE_GIT_TAG) + export SOURCE_GIT_COMMIT="$(params.SOURCE_GIT_COMMIT)" + export SOURCE_GIT_TAG="$(params.SOURCE_GIT_TAG)"
250-258: Confirm workspace and dependency flow for prefetch-dependencies.The prefetch-dependencies task now correctly runs after update-version (line 258), ensuring that package.json is updated before dependency prefetching. The workspace binding is appropriate.
Makefile (1)
14-14: Approve IMAGE_TAG derivation from git HEAD.Line 14 correctly computes IMAGE_TAG from the current HEAD commit using
git rev-parse "HEAD^{commit}". The dereference syntax (^{commit}) ensures that annotated tags resolve to their underlying commit, which is good for consistency. The fallback to empty string on git errors is acceptable..tekton/migration-planner-ui-tag.yaml (1)
25-25: Verify output-image tag change from {{git_tag}} to {{revision}}.Line 25 changes the output-image tag from
{{git_tag}}to{{revision}}. This aligns better with Tekton's template variables and should work correctly for tag events (where revision would be the tag reference). Confirm this change:
- Is
{{revision}}properly set by your Tekton pipeline triggers?- Does this align with your image tagging strategy?
nirarg
force-pushed
the
version-konflux
branch
from
64e1cf2 to
5116b11
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.tekton/migration-planner-ui-push.yaml (1)
193-249: ** Extract commonupdate-versiontask to eliminate duplication.**This task is identical to the one in
.tekton/migration-planner-ui-pull-request.yaml(lines 196–252). See prior comment for refactoring suggestion.
🧹 Nitpick comments (1)
.tekton/migration-planner-ui-pull-request.yaml (1)
196-252: Extract commonupdate-versiontask to eliminate duplication.This inline task definition is duplicated identically in
.tekton/migration-planner-ui-push.yaml(lines 193–249) and.tekton/migration-planner-ui-tag.yamlif it exists. Maintaining two or more copies increases maintenance burden and the risk of drift.Consider extracting this as a shared Tekton
Taskresource or using Tekton's task composition features to define it once.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.tekton/migration-planner-ui-pull-request.yaml(1 hunks).tekton/migration-planner-ui-push.yaml(1 hunks).tekton/migration-planner-ui-tag.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .tekton/migration-planner-ui-tag.yaml
🔇 Additional comments (1)
.tekton/migration-planner-ui-pull-request.yaml (1)
196-252: All concerns verified—script implementation is correct.The
hack/version.shscript exists and properly implements all required functionality:
- ✓ Script exists at repository root with complete implementation
- ✓
--update-package-jsonflag is handled correctly (lines 52–53)- ✓ Reads
SOURCE_GIT_COMMITandSOURCE_GIT_TAGenvironment variables with sensible fallbacks (lines 20–21)- ✓ Uses
jqto safely update bothuiVersionNameanduiGitCommitfields inpackage.json(lines 37–38)- ✓ Atomic file write via temp file to prevent corruption (line 41)
- ✓ Includes jq availability check with proper error handling (lines 32–34)
The Tekton task correctly exports the environment variables before invoking the script, and the script will read them as expected.
| workingDir: $(workspaces.source.path)/source | ||
| script: | | ||
| #!/bin/bash | ||
| set -e | ||
| # Install jq if not available | ||
| microdnf install -y jq | ||
| echo "===== Version Update - Before =====" | ||
| echo "Current working directory: $(pwd)" | ||
| echo "Files in current directory:" | ||
| ls -la | head -20 | ||
| echo "" | ||
| echo "Current package.json version fields:" | ||
| if [ -f package.json ]; then | ||
| jq '{uiVersionName, uiGitCommit}' package.json | ||
| else | ||
| echo "package.json not found!" | ||
| exit 1 | ||
| fi | ||
| # Run the version script to update package.json | ||
| export SOURCE_GIT_COMMIT=$(params.SOURCE_GIT_COMMIT) | ||
| export SOURCE_GIT_TAG=$(params.SOURCE_GIT_TAG) | ||
| echo "" | ||
| echo "Setting version from parameters:" | ||
| echo " SOURCE_GIT_COMMIT=${SOURCE_GIT_COMMIT}" | ||
| echo " SOURCE_GIT_TAG=${SOURCE_GIT_TAG}" | ||
| echo "" | ||
| bash hack/version.sh --update-package-json | ||
| echo "" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify working directory path matches git-clone task output structure.
The script runs in $(workspaces.source.path)/source (line 215), which assumes the git-clone task creates a source/ subdirectory. Confirm this path is correct for the git-clone task used in this pipeline (lines 169–195).
Check the git-clone task definition to confirm its output workspace structure:
🏁 Script executed:
#!/bin/bash
# Locate the git-clone task definition referenced in this pipeline
# Expected: quay.io/konflux-ci/tekton-catalog/task-git-clone:0.1
# The task is defined via bundle reference, but we can search the pipeline
# to understand the workspace mapping
rg -A10 "name: clone-repository" .tekton/migration-planner-ui-pull-request.yaml | head -20Length of output: 231
I need to gather more information about the workspace configuration. Let me expand the search to see the complete task definition and workspace mappings.
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the complete clone-repository task with full context
rg -A20 "name: clone-repository" .tekton/migration-planner-ui-pull-request.yaml
# Also search for workspace definitions in the pipeline
echo "===== Searching for workspace definitions ====="
rg -B5 -A10 "workspaces:" .tekton/migration-planner-ui-pull-request.yaml | head -40
Length of output: 1727
I need to see the complete workspace configuration for the update-version task (the one containing the script). Let me search for that.
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the complete update-version task definition
rg -A30 "name: update-version" .tekton/migration-planner-ui-pull-request.yaml | grep -A25 "workspaces:"
# Also check if there are any variables or workspace definitions at pipeline level
echo "===== Pipeline-level workspaces ====="
rg -B2 -A15 "^spec:" .tekton/migration-planner-ui-pull-request.yaml | grep -A10 "workspaces:"
Length of output: 581
Let me search for the complete workspace binding configuration between tasks.
---
🏁 Script executed:
```shell
#!/bin/bash
# Look for complete task definition linking and workspace flow
rg -B5 -A15 "name: clone-repository" .tekton/migration-planner-ui-pull-request.yaml | grep -A8 "workspaces:"
# Search for how update-version gets its source workspace
rg -B10 "name: update-version" .tekton/migration-planner-ui-pull-request.yaml | tail -20
# Look at complete pipeline spec for workspace declarations
echo "===== Full pipeline spec excerpt ====="
rg -A50 "apiVersion: tekton.dev" .tekton/migration-planner-ui-pull-request.yaml | head -60
Length of output: 2565
Let me find the workspace definitions in the pipelineSpec section.
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the workspaces section from pipelineSpec
rg "pipelineSpec:" .tekton/migration-planner-ui-pull-request.yaml -A200 | grep -A20 "workspaces:" | head -30
# Also check if there are workspace definitions at the top level PipelineRun spec
rg "^spec:" .tekton/migration-planner-ui-pull-request.yaml -A100 | grep -B5 -A15 "workspaces:"
Length of output: 1584
Let me search more specifically for the complete update-version task definition including its workspace binding.
---
🏁 Script executed:
```shell
#!/bin/bash
# Get complete update-version task with workspace binding
rg -A50 "name: update-version" .tekton/migration-planner-ui-pull-request.yaml | grep -B2 -A10 "workspaces:"
# Also look for any workspace named "source" in the pipeline
rg "name: source" .tekton/migration-planner-ui-pull-request.yaml -B3 -A3
Length of output: 1311
Let me verify the standard behavior of the git-clone task used in this pipeline.
---
🌐 Web query:
```web
Konflux-ci task-git-clone 0.1 output workspace structure
💡 Result:
The Konflux CI git-clone task (task-git-clone 0.1 / git-clone-oci-ta) places the fetched repository into the bound output workspace root (or into a subdirectory when the task’s subdirectory/SUBDIRECTORY parameter is set). The workspace will contain the repository files and a .git directory; the task also exposes results such as the commit SHA and repository URL (e.g., COMMIT, URL, COMMITTER_DATE). (konflux-ci.dev)
If you want the exact task YAML or the task bundle name used by Konflux (e.g. task-git-clone-oci-ta:0.1) I can fetch that file and show the workspace mount and parameter names.
Citations:
Fix working directory path in update-version task.
The script uses workingDir: $(workspaces.source.path)/source, but the git-clone task from Konflux outputs the repository directly to the workspace root (not a source/ subdirectory). The Konflux CI git-clone task places the fetched repository into the bound output workspace root, and no subdirectory parameter is configured in the clone-repository task.
Change line 215 to:
workingDir: $(workspaces.source.path)
Or verify the clone-repository task is configured with a SUBDIRECTORY parameter if the source/ subdirectory structure is intentional.
🤖 Prompt for AI Agents
In .tekton/migration-planner-ui-pull-request.yaml around lines 215 to 246, the
task sets workingDir to $(workspaces.source.path)/source but the Konflux
git-clone writes the repo to the workspace root (no source/ subdirectory);
change the workingDir to $(workspaces.source.path) or alternatively update the
clone-repository task to output into a source/ subdirectory (e.g. add/configure
a SUBDIRECTORY parameter) so the script runs in the correct directory.
5116b11 to
b08f441
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
hack/version.sh (1)
29-49: Consider adding package.json existence check.The
update_package_jsonfunction correctly checks forjqand uses atomic file replacement with a temp file. However, it assumesPACKAGE_JSONexists.Consider adding an existence check at the start of the function:
update_package_json() { echo "Updating ${PACKAGE_JSON} with version information..." + # Check if package.json exists + if [ ! -f "${PACKAGE_JSON}" ]; then + echo "Error: ${PACKAGE_JSON} not found." + exit 1 + fi + # Check if jq is installed if ! command -v jq &> /dev/null; then echo "Error: jq is required to update package.json. Please install jq." exit 1 fiNote: The Tekton pipeline task already checks for package.json existence (line 222-227 of migration-planner-ui-push.yaml), so this is mainly for local usage safety.
Makefile (1)
41-41: LGTM! Centralized version script sourcing.Sourcing
hack/version.shbefore npm commands is cleaner than previous per-target environment variable exports. Thebash -cwrapper ensures that the sourced environment variables are available to the npm commands.For robustness, consider adding a check that the version script exists:
build-standalone: install @echo "Building standalone application..." rm -rf dist-standalone - @bash -c '. ./hack/version.sh && npm run build:standalone' + @bash -c 'test -f ./hack/version.sh || { echo "Error: hack/version.sh not found"; exit 1; }; . ./hack/version.sh && npm run build:standalone' @echo "✅ Standalone build completed in dist-standalone/"However, this adds complexity and the current approach will fail fast if the script is missing (due to the
.command failing), so this is purely optional.Also applies to: 48-48, 55-55
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.tekton/migration-planner-ui-branch-push.yaml(0 hunks).tekton/migration-planner-ui-pull-request.yaml(1 hunks).tekton/migration-planner-ui-push.yaml(1 hunks).tekton/migration-planner-ui-tag.yaml(2 hunks)Makefile(2 hunks)hack/version.sh(1 hunks)package.json(1 hunks)src/common/version.ts(2 hunks)
💤 Files with no reviewable changes (1)
- .tekton/migration-planner-ui-branch-push.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- .tekton/migration-planner-ui-pull-request.yaml
- package.json
- .tekton/migration-planner-ui-tag.yaml
🔇 Additional comments (11)
src/common/version.ts (2)
4-5: LGTM! Import path is correct.The relative import path
../../package.jsoncorrectly resolves fromsrc/common/version.tsto the rootpackage.json.
37-39: Verification complete — all criteria satisfied.The code correctly implements the fallback chain:
buildManifestis properly imported frompackage.jsonwith TypeScript support (resolveJsonModule enabled in tsconfig.json)- Both
uiVersionNameanduiGitCommitfields are present inpackage.json- Functions at lines 37-39 and 47-49 correctly reference these fields as fallbacks to environment variables
- Type safety is ensured for the imported JSON object
hack/version.sh (4)
1-14: LGTM! Proper bash script setup.The shebang, error handling flags (
set -e -u -o pipefail), and documentation are appropriate for a version computation script.
16-18: LGTM! Correct path resolution.Using
BASH_SOURCE[0]andcdwithpwdto determine script and repository paths is reliable.
21-22: LGTM! Robust git information extraction.The fallback chain using environment variables (
SOURCE_GIT_COMMIT,SOURCE_GIT_TAG) with git commands and final'unknown'fallbacks handles various execution contexts well (Tekton pipelines, local development, non-git directories).
52-58: LGTM! Clean execution vs. sourcing logic.The check using
BASH_SOURCE[0]vs$0correctly distinguishes between sourcing (for environment variable export) and execution (for command-line usage). The argument handling for--update-package-jsonis straightforward.Makefile (1)
14-14: LGTM! Direct git-based IMAGE_TAG derivation.The change from
$(SOURCE_GIT_COMMIT)to$(shell git rev-parse "HEAD^{commit}" 2>/dev/null)directly derives the image tag from the current git commit. The^{commit}suffix ensures dereferencing of tag objects, and2>/dev/nullsuppresses errors in non-git contexts..tekton/migration-planner-ui-push.yaml (4)
193-201: LGTM! Proper task integration.The
update-versiontask is correctly positioned in the pipeline flow: it runs afterclone-repositoryand beforeprefetch-dependencies. The parameters pass git commit and tag information from the clone step, which aligns with the version script's requirements.
210-227: LGTM! Good error handling and logging.The script installation of
jq, existence check forpackage.json, and before/after logging provide good observability and fail-fast behavior. Theset -eflag ensures that any failure in the chain will stop execution.
229-237: LGTM! Proper environment variable passing.The script correctly exports
SOURCE_GIT_COMMITandSOURCE_GIT_TAGfrom task parameters before invokinghack/version.sh --update-package-json. This ensures the version script uses Tekton's git information rather than attempting git commands (which might not work in the container context).
250-251: LGTM! Correct dependency update.The
prefetch-dependenciestask now runs afterupdate-versioninstead of directly afterclone-repository. This ensures that the updatedpackage.json(with version information) is present before dependency prefetching, which is the correct order.
nirarg
force-pushed
the
version-konflux
branch
from
b08f441 to
155fcfb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.tekton/migration-planner-ui-pull-request.yaml (1)
215-215: Fix incorrect working directory path (duplicate issue).Same as
.tekton/migration-planner-ui-push.yaml- the workingDir must be corrected to point to workspace root instead of$(workspaces.source.path)/source.- workingDir: $(workspaces.source.path)/source + workingDir: $(workspaces.source.path).tekton/migration-planner-ui-tag.yaml (1)
212-212: Fix incorrect working directory path (duplicate issue).Same as the push and pull-request pipelines - correct the workingDir to point to workspace root.
- workingDir: $(workspaces.source.path)/source + workingDir: $(workspaces.source.path)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.tekton/ec-policy.yaml(1 hunks).tekton/migration-planner-ui-branch-push.yaml(0 hunks).tekton/migration-planner-ui-pull-request.yaml(1 hunks).tekton/migration-planner-ui-push.yaml(1 hunks).tekton/migration-planner-ui-tag.yaml(2 hunks)Makefile(2 hunks)hack/version.sh(1 hunks)package.json(1 hunks)src/common/version.ts(2 hunks)
💤 Files with no reviewable changes (1)
- .tekton/migration-planner-ui-branch-push.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- hack/version.sh
- src/common/version.ts
- Makefile
🔇 Additional comments (4)
package.json (1)
3-4: LGTM!The new version fields are properly initialized with sensible defaults and will be updated by the Tekton pipeline task.
.tekton/ec-policy.yaml (1)
1-21: Good alignment with versioning workflow.The policy correctly excludes the custom update-version task from Enterprise Contract checks, with clear documentation of the rationale. This supports the new Konflux-integrated versioning approach.
.tekton/migration-planner-ui-tag.yaml (1)
25-25: Verify impact of output-image tag change on release tagging.Line 25 changes the output-image parameter from
{{git_tag}}to{{revision}}. Verify this change:
- Does not break release image tagging logic
- Aligns with how the tag pipeline is triggered (based on
refs/tags/per line 10)- Is intentional as part of the versioning workflow changes
The commit expression triggering the tag pipeline uses
target_branch.startsWith("refs/tags/"), but the image tag now uses{{revision}}instead of{{git_tag}}. Ensure this semantic change is intentional and documented..tekton/migration-planner-ui-push.yaml (1)
193-248: No changes required; implementation verified.The hack/version.sh script exists and correctly implements all specified requirements:
- Accepts the
--update-package-jsonflag- Reads SOURCE_GIT_COMMIT and SOURCE_GIT_TAG environment variables
- Updates package.json fields uiVersionName and uiGitCommit using jq
- Includes proper error handling with
set -o errexitto ensure correct exit status
| steps: | ||
| - name: update-package-json | ||
| image: registry.access.redhat.com/ubi9/ubi-minimal:latest | ||
| workingDir: $(workspaces.source.path)/source |
There was a problem hiding this comment.
Critical: Fix incorrect working directory path.
The workingDir path assumes git-clone outputs to a source/ subdirectory, but the Konflux git-clone task (0.1) outputs the repository directly to the workspace root. This will cause the package.json not found! error at line 225.
Change line 212 from:
workingDir: $(workspaces.source.path)/sourceTo:
workingDir: $(workspaces.source.path)This applies to all three pipeline YAML files (.tekton/migration-planner-ui-push.yaml, .tekton/migration-planner-ui-pull-request.yaml, .tekton/migration-planner-ui-tag.yaml).
- workingDir: $(workspaces.source.path)/source
+ workingDir: $(workspaces.source.path)Also applies to: 215-215
🤖 Prompt for AI Agents
In .tekton/migration-planner-ui-push.yaml around lines 212 and 215, the
workingDir incorrectly appends "/source" (workingDir:
$(workspaces.source.path)/source) while the Konflux git-clone task places the
repo at the workspace root; change both occurrences to use the workspace root
(workingDir: $(workspaces.source.path)). Apply the same fix to the corresponding
lines in .tekton/migration-planner-ui-pull-request.yaml and
.tekton/migration-planner-ui-tag.yaml so all three pipeline YAMLs use
$(workspaces.source.path) as the workingDir.
Add default value in package.json file and update the file before konflux build This was done this way because this project used the Dockerfile from build-tools Signed-off-by: Nir Argaman <[email protected]>
nirarg
force-pushed
the
version-konflux
branch
from
155fcfb to
2b6df8b
Compare
1 participant
Add default value in package.json file and update the file before konflux build This was done this way because this project used the Dockerfile from build-tools
Summary by CodeRabbit
New Features
Chores