Konflux-build-cli: replace build step of build-image-index task#3321
Konflux-build-cli: replace build step of build-image-index task#3321mkosiarc wants to merge 3 commits intokonflux-ci:mainfrom
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Related pr konflux-ci/konflux-build-cli#69 |
91100b5 to
8058d27
Compare
|
/retest |
1 similar comment
|
/retest |
| was not useful. | ||
| - Image reference validation is now stricter and will fail earlier for invalid formats. | ||
|
|
||
| ## Action from Users |
There was a problem hiding this comment.
don't we need migration script?
There was a problem hiding this comment.
We don't strictly speaking need it, since unused params are just ignored by Tekton. But it would be nice to have one
There was a problem hiding this comment.
added migration script
8058d27 to
a875152
Compare
|
IMO we want also this task to be bumped into v3 (it will be just generated) https://github.com/konflux-ci/build-definitions/tree/main/task/build-image-index-min |
10d49b0 to
bfab648
Compare
bumped |
bfab648 to
b902c89
Compare
| @@ -0,0 +1,25 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
This mgiration scrip is in min task, but we need it in the regular task
There was a problem hiding this comment.
Forgot to git add in the regular task, updated.
chmeliik
left a comment
There was a problem hiding this comment.
I think the way we currently pass boolean params is wrong, would be worth manually testing.
Minor comment on the commit message:
Also NOTE!: The build step now needs to be run as root, since the
permissions for the "konflux-build-cli" container are different
This sounds worse than it is - the step always needed to run as root. Previously, root was the default user in the container image, now we just need to set root explicitly
| "--tls-verify" "$TLSVERIFY" | ||
| "--buildah-format" "$BUILDAH_FORMAT" | ||
| "--always-build-index" "$ALWAYS_BUILD_INDEX" |
There was a problem hiding this comment.
I think all the boolean flags need to be in this form: --tls-verify=$TLSVERIFY. Related: konflux-ci/konflux-build-cli#60
I think e2e tests are currently not testing version 0.3. We need to update the version in pipelines/
There was a problem hiding this comment.
updated how I pass the booleans + updated the pipelines now
| for image in "$@"; do | ||
| cli_args+=("--images" "$image") | ||
| done |
There was a problem hiding this comment.
nitpick: this can just be --images "$@", and we don't even need the cli_args array
There was a problem hiding this comment.
updated and removed the cli_args completely
b902c89 to
6f3b6eb
Compare
6f3b6eb to
49a22c3
Compare
You are right. Updated the commit message to not sound so alarming and explicitly mentioned that nothing really changed |
Start using the konflux-build-cli for building the image index. Related PR: konflux-ci/konflux-build-cli#69 Also NOTE: The build step is now run explicitly as root by using runAsUser: 0, since the permissions for the "konflux-build-cli" container are different and they no longer allow executing the necessary setup steps, like updating the ca-trust. The step was always run with root permissions, now it us just explicitly setup in the tekton step. STONEBLD-4060 Assisted-by: Claude Signed-off-by: mkosiarc <[email protected]>
49a22c3 to
9dc517a
Compare
Start using the konflux-build-cli for building the image index.
Related PR: konflux-ci/konflux-build-cli#69
Also NOTE: The build step is now run explicitly as root by using
runAsUser: 0, since the permissions for the "konflux-build-cli" container
are different and they no longer allow executing the necessary setup steps,
like updating the ca-trust. The step was always run with root
permissions, now it us just explicitly setup in the tekton step.
STONEBLD-4060