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

Add optional platform argument to ContainerBase when docker pull is executed #183

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jathanism
Copy link

  • Also added optional platform argument to ContainerLauncher.launch_container when docker run is executed.
  • Also fix a bug in runtime to fix a crash when CONTAINER_RUNTIME environment isn't set but Docker is installed. It will now check for Docker or Podman before attempting to default to Podman or raising an error.
  • Also removed Python 3.6-3.9 support from testing, because they are deprecated.
  • Unrelated changes due to making Black lint check pass

- Also fix a bug in runtime to fix a crash when environment isn't set but Docker is installed.
@dcermak
Copy link
Owner

dcermak commented Jan 29, 2024

Thanks for the PR! I'll take a closer look but I'll be traveling next week so please bear with me.

However, I cannot merge the removal of the EoL python versions. Sadly I still need to support Python 3.6 onwards (oh the joys of enterprise distributions). Thus please revert this specific part of your PR.


# Construct the args passed to the Docker CLI
cmd_args = [runtime.runner_binary, "pull"]
if self.platform is not None:
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
if self.platform is not None:
if self.platform:

Copy link

codecov bot commented Jan 29, 2024

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (cc6b42a) 94.53% compared to head (b0441d3) 92.92%.

Files Patch % Lines
pytest_container/container.py 50.00% 2 Missing and 2 partials ⚠️
pytest_container/runtime.py 66.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #183      +/-   ##
==========================================
- Coverage   94.53%   92.92%   -1.61%     
==========================================
  Files           9        9              
  Lines        1079     1088       +9     
  Branches      224      220       -4     
==========================================
- Hits         1020     1011       -9     
- Misses         45       57      +12     
- Partials       14       20       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dcermak
Copy link
Owner

dcermak commented Jan 30, 2024

This will require quite a bit more effort. The current PR only works for docker and it does not handle derived containers properly.

I'll try to implement this, but I'd be interested first if all you need is to set the architecture or you really need to set the full platform string (i.e. os + arch)?

@shakefu
Copy link

shakefu commented Feb 7, 2024

I'll try to implement this, but I'd be interested first if all you need is to set the architecture or you really need to set the full platform string (i.e. os + arch)?

The use case we’re trying to address is running linux/amd64 images on Docker Desktop on OSX M* which works only for the full OS + architecture specifier (otherwise it would be darwin/amd64 which is not a Docker platform supported on the OSX Docker VM).

@jathanism
Copy link
Author

jathanism commented Feb 7, 2024

This will require quite a bit more effort. The current PR only works for docker and it does not handle derived containers properly.

I'll try to implement this, but I'd be interested first if all you need is to set the architecture or you really need to set the full platform string (i.e. os + arch)?

Thanks for the response! We were hacking on this for some private code and were having problems with environments that support multi-platform builds and requiring the target architecture was the only way around to get both pull and run to work.

I honestly don't need the credit. Truly the sentiment here is just adding support for the --platform argument. A seemingly simple fix with a lot of side effects apparently.

Good question on the os + arch. Tried setting platform="amd64" and it failed. So it appears it wants both.

@shakefu
Copy link

shakefu commented Feb 7, 2024

Just to add more context the reason for dropping support for earlier versions is pytest-testinfra which is leveraged extensively doesn’t implement Darwin support for one of the host resources until 10.x, which drops support for <3.8 iirc. Not sure that’s surmountable. Happy to do local OSX testing though if you get a possible solution which isn’t testable on CI.

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