-
Notifications
You must be signed in to change notification settings - Fork 140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(MMENG-4237): add build-maven-zip task #1643
Conversation
d151d5d
to
32ab159
Compare
set -e | ||
# Generate checksums for all maven artifact files. It will ignore the checksum files | ||
# and signature files if they existed there | ||
BYPASS_FILE_TYPES=("md5" "sha1" "sha128" "sha256" "sha512" "asc") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use Bash Associative arrays to implement the bypass? It's more like operating a dict in other programming language, which would make the code more readable than using flag+loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This BYPASS has been removed in commit. Instead the logic is implemented through "find" directly.
cpu: '1' | ||
script: | | ||
#!/bin/bash | ||
set -e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add set -o pipefail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
oras push "$IMAGE" \ | ||
--registry-config auth.json \ | ||
"${EXPIRE_LABEL[@]}" \ | ||
--artifact-type application/vnd.maven+zip "${BUNDLE_NAME}.zip" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upload the maven zip to quay.io as OCI-artifact
Upload the zip package as a standalone artifact or attach it to the image built by the build-container task?
Env var IMAGE
is described in the spec.params
as following:
Reference of the image buildah will produce.
But, this oras push
pushes the zip package to the registry as image $IMAGE
.
I guess you probably need oras attach
to attach the zip package to the image $IMAGE
.
Or, does the param description need to be changed?
By the way, for easier-to-read, please indent the continuous lines with at least 2 spaces. 2 is preferred personally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new pipeline will not generate any images there, so this task will directly push the zip file as oci-artifact. This IMAGE param is used here because the pipeline will use IMAGE to pass the registry name which we can not change, so I'd prefer to leave it here, but I'd like to change the description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indent changed in new commit.
mv sbom-cachi2.json sbom-cyclonedx.json | ||
echo "Creating sbom-purl.json" | ||
python3 /scripts/create_purl_sbom.py | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous step https://github.com/konflux-ci/build-definitions/pull/1643/files#diff-6199ec974f25eac323954ab607a9160cb1fe2a523abf0e255775588e1e2bda01R136 has already ensured the existence of file ./sbom-cachi2.json
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a double insurance, and I think it does not break anything, so I'd like to keep it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But, the logic looks ambiguous. In previous step, if the bom.json does not exist, it is an error and will fail the task, then no cachi2-sbom.json is created, then perhaps fail the pipeline, but in this step, the sbom-cachi2.json
becomes optional for this build task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I've removed it in new commit.
716fb3e
to
ee22e6c
Compare
/ok-to-test |
6283319
to
b35722b
Compare
task/build-maven-zip/0.1/README.md
Outdated
| -------------------- | ---------------------------------------------------------------------- | ---------------- | -------- | | ||
| IMAGE | Reference of the OCI-Artifact this build task will produce. | | true | | ||
| PREFETCH_ROOT | The root directory of the artifacts in the prefetched directory. | maven-repository | false | | ||
| BUNDLE_NAME | The zip bundle name of archived artifacts. | maven-repository | false | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| BUNDLE_NAME | The zip bundle name of archived artifacts. | maven-repository | false | | |
| FILE_NAME | The zip bundle name of archived artifacts. | maven-repository.zip | false | |
^ would feel less magical to me, but I'm not the intended user so I'll leave this to you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not any prefer from my side. Ok to change. But the default name should not include .zip there because it is handled in the task logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed in new commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the default name should not include .zip there because it is handled in the task logic.
Ah, that's what I meant - that the parameter should be the whole filename, not just the part before the task adds .zip
. It's just clearer what the parameter means that way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise, BUNDLE_NAME
was probably better, FILE_NAME
would just be misleading
9fd5790
to
2c9ced9
Compare
/retest |
1 similar comment
/retest |
/verify-owners |
computeResources: | ||
limits: | ||
memory: 8Gi | ||
cpu: '4' | ||
requests: | ||
memory: 2Gi | ||
cpu: '1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, that's way too much just to run oras
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how much memory will oras cost to upload a 2GB+ file. If we can make sure the file size will not bring big impact on oras upload, we can reduce it to 1Gi.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure it won't have an impact, but OK, let's keep 2
The MRRC maven build pipeline for the whole MRRC release process is developed done in following PRs: * konflux-ci/build-definitions#1643 * konflux-ci/build-definitions#1691 We want to deploy these two pipelines to the kflux-prd-rh02 for user to do early use. This PR addressing the deployment. Signed-off-by: Gang Li <[email protected]>
The MRRC maven build pipeline for the whole MRRC release process is developed done in following PRs: * konflux-ci/build-definitions#1643 * konflux-ci/build-definitions#1691 We want to deploy these two pipelines to the kflux-prd-rh02 for user to do early use. This PR addressing the deployment. Signed-off-by: Gang Li <[email protected]>
The MRRC maven build pipeline for the whole MRRC release process is developed done in following PRs: * konflux-ci/build-definitions#1643 * konflux-ci/build-definitions#1691 We want to deploy these two pipelines to the kflux-prd-rh02 for user to do early use. This PR addressing the deployment. Signed-off-by: Gang Li <[email protected]>
The MRRC maven build pipeline for the whole MRRC release process is developed done in following PRs: * konflux-ci/build-definitions#1643 * konflux-ci/build-definitions#1691 We want to deploy these two pipelines to the kflux-prd-rh02 for user to do early use. This PR addressing the deployment. Signed-off-by: Gang Li <[email protected]>
The MRRC maven build pipeline for the whole MRRC release process is developed done in following PRs: * konflux-ci/build-definitions#1643 * konflux-ci/build-definitions#1691 We want to deploy these two pipelines to the kflux-prd-rh02 for user to do early use. This PR addressing the deployment. Signed-off-by: Gang Li <[email protected]>
The MRRC maven build pipeline for the whole MRRC release process is developed done in following PRs: * konflux-ci/build-definitions#1643 * konflux-ci/build-definitions#1691 We want to deploy these two pipelines to the kflux-prd-rh02 for user to do early use. This PR addressing the deployment. Signed-off-by: Gang Li <[email protected]>
This task will be used in the new Konflux MRRC maven build pipeline. It is responsible for these work:
It also includes the oci-ta version.
BTW, we've added a new owner alias to put our team members in, and also mark the owners of this task as our team for future update.