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

conn: use /usr/bin/env to run the shell #96

Merged
merged 3 commits into from
Nov 28, 2024

Conversation

pbrezina
Copy link
Contributor

To make the code more portable to distributions that don't have bash in
/usr/bin.

@alejandro-colomar
Copy link

Cc: @ikerexxe

For commit 2 (where the meat is), please add:

Link: <https://github.com/shadow-maint/shadow/pull/1131>
Reported-by: Iker Pedrosa <[email protected]>
Reported-by: Alejandro Colomar <[email protected]>
Suggested-by: Alejandro Colomar <[email protected]>

The patch LGTM. Thanks!

To make the code more portable to distributions that don't have
bash in /usr/bin.

Resolves: next-actions#95

Link: <shadow-maint/shadow#1131>
Reported-by: Iker Pedrosa <[email protected]>
Reported-by: Alejandro Colomar <[email protected]>
Suggested-by: Alejandro Colomar <[email protected]>
The original link is no longer available.
@pbrezina
Copy link
Contributor Author

Cc: @ikerexxe

For commit 2 (where the meat is), please add:

Link: <https://github.com/shadow-maint/shadow/pull/1131>
Reported-by: Iker Pedrosa <[email protected]>
Reported-by: Alejandro Colomar <[email protected]>
Suggested-by: Alejandro Colomar <[email protected]>

Done

Copy link
Contributor

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

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

There's another mention of /usr/bin/bash -c in pytest_mh/_private/multihost.py, it's a comment so it wouldn't be a big deal to forget about it, but I'd prefer to update it and have everything aligned.

Apart from that it looks good. Will you be able to generate a new release after these changes are merged? I'd like to pin a specific version of pytest-mh in shadow.

@ikerexxe
Copy link
Contributor

There's another mention of /usr/bin/bash -c in pytest_mh/_private/multihost.py, it's a comment so it wouldn't be a big deal to forget about it, but I'd prefer to update it and have everything aligned.

It was already removed in the first commit 🤦

Copy link
Contributor

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

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

LGTM!

@pbrezina pbrezina merged commit c5a2469 into next-actions:master Nov 28, 2024
4 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