Skip to content

Fix support of inline scripts in prepare/shell#4875

Open
thrix wants to merge 1 commit into
mainfrom
fix-multiline-script-image-mode
Open

Fix support of inline scripts in prepare/shell#4875
thrix wants to merge 1 commit into
mainfrom
fix-multiline-script-image-mode

Conversation

@thrix
Copy link
Copy Markdown
Contributor

@thrix thrix commented May 7, 2026

An inline script in prepare/shell can break easily the RUN directive. We run into this when working on profiles where we had a script with a function:

https://gitlab.com/testing-farm/profiles/-/merge_requests/85/diffs

To fix this use RUN heredoc to properly support any scripts user can throw on us.

Resolves #4868

Pull Request Checklist

  • implement the feature
  • extend the test coverage
  • include a release note

@thrix thrix requested review from LecrisUT and lukaszachy May 7, 2026 15:55
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request enables support for multiline shell scripts in Containerfiles by utilizing heredocs for command collection. A bug was identified in the test suite where the assertion string does not match the actual output of the multiline test script.

@teemtee teemtee deleted a comment from graphite-app Bot May 7, 2026
@teemtee teemtee deleted a comment from gemini-code-assist Bot May 7, 2026
An inline script in `prepare/shell` can break easily
the RUN directive. We run into this when working on
profiles where we had a script with a function:

https://gitlab.com/testing-farm/profiles/-/merge_requests/85/diffs

To fix this use RUN heredoc to properly support any scripts
user can throw on us.

Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
@thrix thrix force-pushed the fix-multiline-script-image-mode branch from 214b848 to b3e7329 Compare May 7, 2026 16:48
@thrix thrix added the ci | full test Pull request is ready for the full test execution label May 7, 2026
@thrix
Copy link
Copy Markdown
Contributor Author

thrix commented May 7, 2026

/packit build

@happz
Copy link
Copy Markdown
Contributor

happz commented May 7, 2026

@thrix is there an issue? I recall several issues filed today related to continerfiles, namely #4868 and #4867, plus one extra PR, #4871. I'd like to be sure you and @lukaszachy are aligned, you seem to be touching same code.

@lukaszachy
Copy link
Copy Markdown
Contributor

@thrix is there an issue? I recall several issues filed today related to continerfiles, namely #4868 and #4867, plus one extra PR, #4871. I'd like to be sure you and @lukaszachy are aligned, you seem to be touching same code.

From what I tried this PR seems to fix #4868 (multiline problem) but not #4867 (variable expansion)

)
self.guest.execute(
ShellScript(f'cat <<EOF > {containerfile_path!s} \n{containerfile} \nEOF')
ShellScript(f'cat <<EOF > {containerfile_path!s}\n{containerfile}\nEOF')
Copy link
Copy Markdown
Member

@LecrisUT LecrisUT May 8, 2026

Choose a reason for hiding this comment

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

Iiuc what you want here is textwrap.deindent? Will look at it again when I have a fresh mind

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

not sure, I will take a look

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok, new day fresh mind. Why not guest.push these? There are so many other edge cases, e.g. what if the containerfile_directives contains

RUN cat <<EOF > /etc/profile.d/mpi.sh
module load mpi
EOF

looks like it will clash and end in a fun state.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I will try, not originally my code, it was just causing me trouble due to the used spurious spaces.

@thrix
Copy link
Copy Markdown
Contributor Author

thrix commented May 11, 2026

@thrix is there an issue? I recall several issues filed today related to continerfiles, namely #4868 and #4867, plus one extra PR, #4871. I'd like to be sure you and @lukaszachy are aligned, you seem to be touching same code.

I did not file an issue, we found this on Thursday while working on Image Mode in profiles

@thrix
Copy link
Copy Markdown
Contributor Author

thrix commented May 11, 2026

@thrix is there an issue? I recall several issues filed today related to continerfiles, namely #4868 and #4867, plus one extra PR, #4871. I'd like to be sure you and @lukaszachy are aligned, you seem to be touching same code.

I did not file an issue, we found this on Thursday while working on Image Mode in profiles

This should fix #4868. The #4867 & #4871 I would keep as separate, it is another separate problem

@thrix thrix added this to planning May 13, 2026
@github-project-automation github-project-automation Bot moved this to backlog in planning May 13, 2026
@thrix thrix moved this from backlog to implement in planning May 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci | full test Pull request is ready for the full test execution

Projects

Status: implement

Development

Successfully merging this pull request may close these issues.

Multiline prepare script breaks containerfile in image mode

4 participants