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

Send optional platform spec when creating container #395

Merged

Conversation

hairyhenderson
Copy link
Contributor

This is based on #342, and adds support for explicitly specifying image platform (like docker run --platform=...).

I'm opening a new PR because the other one seemed to be stalled, and I needed this for a project anyway. This differs slightly from #342:

  • uses containerd's platforms.Parse to parse the platform string instead of strings.Split - this will also handle 3-part platform strings (such as linux/arm/v6)
  • adds a positive test case (without actually starting the container, which is unnecessary)

Obviously having two PRs open for the same thing isn't ideal, and my goal isn't to compete with #342 - just to get things moving again. If @amitsaha wants to take some of these changes and rebase their own PR, I'm happy with this one being closed!

Signed-off-by: Dave Henderson dhenderson@gmail.com

Signed-off-by: Dave Henderson <dhenderson@gmail.com>
@amitsaha
Copy link

amitsaha commented Jan 9, 2022

I am happy for this PR to merged as well. If the maintainers want me to make the changes to my PR, i am happy to do that too.

@hairyhenderson
Copy link
Contributor Author

Hey @mdelapenya @gianarb - sorry to bother you, could I get some eyes on this?

I've been using this commit successfully in some of my own tests for a few days with no issues so far.

@codecov
Copy link

codecov bot commented Jan 13, 2022

Codecov Report

Merging #395 (9ec3ed4) into master (d303350) will increase coverage by 0.29%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #395      +/-   ##
==========================================
+ Coverage   63.98%   64.27%   +0.29%     
==========================================
  Files          18       18              
  Lines        1108     1117       +9     
==========================================
+ Hits          709      718       +9     
  Misses        295      295              
  Partials      104      104              
Impacted Files Coverage Δ
container.go 88.37% <ø> (ø)
docker.go 64.83% <83.33%> (+0.63%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d303350...9ec3ed4. Read the comment docs.

Copy link
Collaborator

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for this contribution! and sorry for the delay in the review, I returned from Xmas PTO past Monday

@hairyhenderson
Copy link
Contributor Author

@mdelapenya no worries, thanks for the review! Is there something I should to to fix the CI failure? It seems to be a linting issue with a file I didn't change... 🤔

@hairyhenderson
Copy link
Contributor Author

@mdelapenya hi! any updates? should I do anything to fix the CI failure?

@mdelapenya
Copy link
Collaborator

@mdelapenya hi! any updates? should I do anything to fix the CI failure?

The CI is complaining about an EOF not properly set. Please take a look at #396

I'd say we better push the format fix in a separate PR so that your code can be rebased/merged on top of that

@hairyhenderson
Copy link
Contributor Author

@mdelapenya I've issued #400 to fix the lint errors - please have a look - I'll rebase this once that one is merged

@gianarb
Copy link
Collaborator

gianarb commented Feb 2, 2022

Thanks a lot @hairyhenderson !!

@gianarb gianarb merged commit a5da993 into testcontainers:master Feb 2, 2022
@hairyhenderson hairyhenderson deleted the add-container-platform-spec branch February 2, 2022 14:15
@hairyhenderson
Copy link
Contributor Author

Thanks @gianarb and @mdelapenya 🙇‍♂️

@mdelapenya mdelapenya added the feature New functionality or new behaviors on the existing one label Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality or new behaviors on the existing one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants