Skip to content

Conversation

@missytake
Copy link
Contributor

Based on #659, this is also pulled out of Keonik1#3. It enables us to run some DNS checks in a docker container; it could also be part of #659, but I put it in a separate PR, as it isn't useful before #614.

@hpk42
Copy link
Contributor

hpk42 commented Oct 8, 2025

I suggest to rename "docker" to ":docker" throughout the PR to disambiguate from regular ssh hosts.
Also it's a bit weird to merge this PR before the actual docker PR, and there are no tests or CI yet.

@missytake
Copy link
Contributor Author

What about @docker? pyinfra uses @local for running on localhost, we support that as well, that would make it a bit more consistent.

I wonder if any of those options could mess with SSH configs...

@hpk42
Copy link
Contributor

hpk42 commented Oct 9, 2025

yes, @docker is better if we have @Local already. i think "ssh @docker" is invalid, so i guess it's ok wrt ssh config. anything with an @ prefix is not an ssh target.

@missytake
Copy link
Contributor Author

there are no tests or CI yet.

The only thing I see that makes sense to test for this PR is whether pre_command is passed correctly to sub-functions. But I fail mocking the shell() function to see the output of a command instead of executing it. Do you have an idea what I'm doing wrong here? Replacing cmdeploy.remote.rshell.shell with test_cmdeploy.test_dns_when_ssh_docker.<locals>.shell seems to work in all methods except the one that counts.

Copy link
Contributor

@hpk42 hpk42 left a comment

Choose a reason for hiding this comment

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

roughly looks good (didn't test it for real) but i recommend to consider the LocalExec/DockerExec suggestion.

@missytake missytake force-pushed the sshexec-rework branch 2 times, most recently from a68963c to cd879bb Compare October 14, 2025 17:23
@missytake missytake merged commit 89ba4b5 into sshexec-rework Oct 14, 2025
6 checks passed
@missytake missytake deleted the sshexec-docker branch October 14, 2025 19:29
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