Skip to content

Conversation

@elfosardo
Copy link
Member

No more && \

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign rozzii for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@metal3-io-bot metal3-io-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 26, 2025
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

Unless heredoc inherits the first set -e, then it'll remove && success chain guarantee, which is changing behavior.

Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

Not sure if we gain anything here, besides the ipxe build part... ?

@elfosardo
Copy link
Member Author

Not sure if we gain anything here, besides the ipxe build part... ?

@tuminoid yeah it's a small change, mostly to remove all the &&
I could add more stuff if you think it's not worth it as it is

@tuminoid
Copy link
Member

Not sure if we gain anything here, besides the ipxe build part... ?

@tuminoid yeah it's a small change, mostly to remove all the && I could add more stuff if you think it's not worth it as it is

I'm just not sure why is it worth replacing a line with && with 5 lines (the last change in diff for example). What is the actual benefit? It is cleaner for the ipxe for sure, as its basically a script (we could put that into a script, like configure-nonroot.sh is) and that'd solve that.

@elfosardo
Copy link
Member Author

I'm afraid podman version in the IrSO functional is too old for heredoc syntax :/

@elfosardo
Copy link
Member Author

Not sure if we gain anything here, besides the ipxe build part... ?

@tuminoid yeah it's a small change, mostly to remove all the && I could add more stuff if you think it's not worth it as it is

I'm just not sure why is it worth replacing a line with && with 5 lines (the last change in diff for example). What is the actual benefit? It is cleaner for the ipxe for sure, as its basically a script (we could put that into a script, like configure-nonroot.sh is) and that'd solve that.

@tuminoid better readability, more clean, possibility to run commands as a script without the need to copy the script in the container and so saving a layer, quicker execution, to mention some
it's also not limited to RUN but also to other statements, I just didn't add them here

@elfosardo
Copy link
Member Author

ubuntu has podman version 4.9 which has some issues parsing the heredoc syntax without adding the shell explicitely, but docker on the other hand uses sh by default breaking the workaround
wondering if we could install at least podman version 5 on ubuntu in some way

@elfosardo
Copy link
Member Author

/test metal3-centos-e2e-integration-test-main metal3-ubuntu-e2e-integration-test-main

@elfosardo
Copy link
Member Author

we should probably switch the IrSO functional to docker on ubuntu and the cs9/cs10 jobs to podman as they have newer versions
I'll do some tests and propose the changes

No more && \

Signed-off-by: Riccardo Pittau <[email protected]>
@elfosardo elfosardo changed the title 🌱 Use heredoc syntax for RUN statements [WIP] Use heredoc syntax for RUN statements Dec 1, 2025
@metal3-io-bot metal3-io-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 1, 2025
@elfosardo
Copy link
Member Author

podman version on noble is just too old to support heredoc, we could use a static compiled version (I tested it and it works) to bump to 5.x to be able to support podman, but we probably need to discuss this first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants