Skip to content
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

Fix non-hermetic OCI TA builds #1043

Merged
merged 1 commit into from
May 30, 2024

Conversation

stuartwdouglas
Copy link
Contributor

No description provided.

@@ -267,7 +267,7 @@ spec:
echo $container > /var/workdir/container_name

# Save the SBOM produced by Cachi2 so it can be merged into the final SBOM later
if [ -d "/var/workdir/cachi2" ]; then
if [ -f "/var/workdir/cachi2/cachi2.env" ]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are checking for files, should we not be checking for output/bom.json instead of cachi2.env?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw the build fail due to a lack of /cachi2/cachi2.env in the dockerfile, so that was the file I checked for. Would it make any functional difference?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does output/bom.json exists == cachi2.env exists? I think so, but why take the risk?

+1 to Andrew's suggestion, let's check the existence of the file we care about.

@@ -359,7 +359,7 @@ spec:
- name: merge-cachi2-sbom
image: quay.io/redhat-appstudio/cachi2:0.7.0@sha256:1fc772aa3636fd0b43d62120d832e5913843e028e8cac42814b487c3a0a32bd8
script: |
if [ -d "/var/workdir/cachi2" ]; then
if [ -f "/var/workdir/cachi2/cachi2.env" ]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, should we just check for the existence of sbom-cachi2.json here?

Copy link
Contributor

@chmeliik chmeliik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update buildah.yaml as well, otherwise #1039 will revert your changes I think

@chmeliik
Copy link
Contributor

Could you explain in the commit message why non-hermetic OCI TA builds were broken and how this fixes them?

@stuartwdouglas stuartwdouglas force-pushed the non-hermetic-oci branch 2 times, most recently from 094c4da to 21c6143 Compare May 29, 2024 23:27
With OCI artifacts the directory is now always there, this now checks
for specific files.
Copy link
Member

@arewm arewm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the changes!

@arewm arewm enabled auto-merge May 29, 2024 23:35
@arewm arewm added this pull request to the merge queue May 29, 2024
Merged via the queue into konflux-ci:main with commit 05c294f May 30, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants