Skip to content

better docker execution #171

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

Merged
merged 6 commits into from
Jan 5, 2023
Merged

better docker execution #171

merged 6 commits into from
Jan 5, 2023

Conversation

bloodearnest
Copy link
Member

  • fix: allow exit codes to pass through
  • fix: handle possible winpty wrapper in RunFixture
  • fix: Add new utils package with run_docker function
  • chore: add a test for the jupyter command.
  • fix: use run_docker with exec command

Also, remove handle_unknown_args hack in favour of argparse.REMAINDER
Also, better AssertionError output for easy side-by-side comparison
The function handles.

 - running an opensafely image
 - with the right args
 - with a directory mounted at /workspace
 - will run as the invoking user on linux by default
 - can handle interactivity and git-bash/winpty

We then use this new function in juypter command. This means it now runs
as the user rather than root, which was an issue when running in linux.

Also, we move from `platform.system()` to `sys.platform`, as the former
calls subprocess.run, which messes with our run fixture expectations,
and we do not need anything beyond what sys.platform provides.
Also adds a helper for doing this, that exercises the argparse code as
well as the main function. This functional approach should help catch
some errors that have slipped through
Removes the execute specific code and uses utils.run_docker. This
re-adds support for interactivity, which had to be removed.

This uses the new argparse test helper for the tests.
Just like docker run, adds `--entrypoint`, `--env`, and `--user`, which
similarly must be specified before the image name.

Also, supply the stata license if it's the stata image.

This involved adding the ability to express the expected env in the
RunFixture
Copy link
Contributor

@evansd evansd left a comment

Choose a reason for hiding this comment

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

👌

# allow functions to return True/False, or an explicit exit code
if success is False:
exit_code = 1
elif success is True:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs or success is None to fully replicate the existing behaviour where functions which don't return anything are assumed successful.

parser.set_defaults(handles_unknown_args=True)
parser.add_argument(
"jupyter_args",
nargs=argparse.REMAINDER,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is much nicer!

@bloodearnest
Copy link
Member Author

Fixes #149

@bloodearnest bloodearnest merged commit 97843d9 into main Jan 5, 2023
@bloodearnest bloodearnest deleted the better-docker-execution branch January 5, 2023 12: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.

2 participants