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 assemble cache clean order in 16-minimal, 18-minimal #459

Merged
merged 1 commit into from
Nov 4, 2024

Conversation

SlouchyButton
Copy link
Contributor

@SlouchyButton SlouchyButton commented Oct 18, 2024

Removal of the cache has to be the last things done in the assemble script. Calling npm after deleting the cache folder will cause npm to recreate it - which will fail during the image test.

Other images apparently have it correctly, for some reason this error is present only in 16-minimal and 18-minimal.

Removal of the cache has to be the last things done in the assemble
script. Calling `npm` after deleting the cache folder will cause npm to
recreate it - which will fail during the image test.
@SlouchyButton
Copy link
Contributor Author

[test]

Copy link

github-actions bot commented Oct 18, 2024

Pull Request validation

Failed

🔴 Failed or pending statuses - Testing Farm - CentOS Stream 10 - 22[error]
🔴 Review - Missing review from a member (2 required)

Success

🟢 Review - Reviewed by undefined

Copy link
Contributor

@lukaszachy lukaszachy left a comment

Choose a reason for hiding this comment

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

LGTM (I'm afraid to click approve in the case it would unblock the merge bot)

@SlouchyButton
Copy link
Contributor Author

SlouchyButton commented Oct 18, 2024

I'm afraid to click approve

afaik two approvals are needed, and it's not yet merging automatically, only sending email that it would.

Copy link
Member

@phracek phracek left a comment

Choose a reason for hiding this comment

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

LGTM, but is it enought to move cleaning operations before NPM_CACHE?

@phracek
Copy link
Member

phracek commented Oct 21, 2024

@lukaszachy @SlouchyButton Automatic merger is not enabled yet;). Don't worry. See send only mails about PR and what is missing. Syncing upstream -> downstream for RHEL-9.5.0 is also disabled.

@SlouchyButton
Copy link
Contributor Author

but is it enought to move cleaning operations before NPM_CACHE

It's hard to test it completely as this issue is present (for some unknown reason) only in the konflux built image, but as far as I tested it, it should resolve the error.

Also, the issue was present only in 16-min and 18-min - this change changes the code to be identical with other version. For some reason, these two containers had the tmp clean and cache clean reversed.

Copy link
Member

@phracek phracek left a comment

Choose a reason for hiding this comment

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

LGTM.

@phracek
Copy link
Member

phracek commented Oct 25, 2024

[test]

Copy link

github-actions bot commented Oct 25, 2024

Testing Farm results

namecomposearchstatusstarted (UTC)timelogs
Fedora - 18-minimalFedora-latestx86_64✅ passed25.10.2024 09:15:2310min 12stest pipeline
Fedora - 18Fedora-latestx86_64✅ passed25.10.2024 09:15:2514min 10stest pipeline
Fedora - 20Fedora-latestx86_64✅ passed25.10.2024 09:17:2715min 26stest pipeline
CentOS Stream 9 - 20CentOS-Stream-9x86_64✅ passed25.10.2024 09:18:3415min 45stest pipeline
RHEL9 - 18-minimalRHEL-9.4.0-Nightlyx86_64✅ passed25.10.2024 09:15:3319min 31stest pipeline
CentOS Stream 10 - 22CentOS-Stream-10x86_64❌ failed04.11.2024 08:47:1110min 48stest pipeline
RHEL9 - 18RHEL-9.4.0-Nightlyx86_64✅ passed25.10.2024 09:15:2721min 3stest pipeline
RHEL8 - 18-minimalRHEL-8.10.0-Nightlyx86_64✅ passed04.11.2024 08:47:1018min 33stest pipeline
RHEL8 - 18RHEL-8.10.0-Nightlyx86_64✅ passed25.10.2024 09:15:2624min 17stest pipeline
CentOS Stream 9 - 20-minimalCentOS-Stream-9x86_64✅ passed25.10.2024 09:21:4219min 10stest pipeline
RHEL9 - 20RHEL-9.4.0-Nightlyx86_64✅ passed25.10.2024 09:21:0420min 36stest pipeline
CentOS Stream 10 - 22-minimalCentOS-Stream-10x86_64✅ passed25.10.2024 09:33:128min 26stest pipeline
RHEL8 - 20RHEL-8.10.0-Nightlyx86_64✅ passed25.10.2024 09:15:3627min test pipeline
Fedora - 20-minimalFedora-latestx86_64✅ passed25.10.2024 09:23:3720min 12stest pipeline
Fedora - 22Fedora-latestx86_64✅ passed25.10.2024 09:30:4913min 16stest pipeline
Fedora - 22-minimalFedora-latestx86_64✅ passed25.10.2024 09:34:5510min 44stest pipeline
RHEL9 - 22-minimalRHEL-9.4.0-Nightlyx86_64✅ passed25.10.2024 09:35:2417min 30stest pipeline
RHEL8 - 20-minimalRHEL-8.10.0-Nightlyx86_64✅ passed04.11.2024 08:47:1326min 28stest pipeline
RHEL9 - 22RHEL-9.4.0-Nightlyx86_64✅ passed04.11.2024 08:47:1521min 26stest pipeline
RHEL9 - 20-minimalRHEL-9.4.0-Nightlyx86_64✅ passed25.10.2024 09:26:1227min 35stest pipeline

@phracek phracek merged commit 8248411 into sclorg:master Nov 4, 2024
20 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants