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

_image_needs_[building|pushing]: check platforms when using docker buildx #136

Merged
merged 4 commits into from
Jul 26, 2021

Conversation

manics
Copy link
Member

@manics manics commented Jul 24, 2021

Closes #132

Not yet tested....


Update by Erik

Tested and it works great!

@manics manics force-pushed the image_needs_pushing_platforms branch 2 times, most recently from ab79814 to 6c2d34e Compare July 24, 2021 20:59
@manics manics force-pushed the image_needs_pushing_platforms branch from 6c2d34e to eb04df3 Compare July 24, 2021 21:02
@consideRatio
Copy link
Member

consideRatio commented Jul 24, 2021

Since we want to work with a frozen set rather than a l list, should we perhaps instead convert the platforms list to a frozen set directly after args = argparser.parse_args(args) and adjust various functions relating to platform?

Or hmm, i see build_images expect a list based on the docs, and is the only recipient of platforms, so perhaps to convert it to a frozenset within build_image.

-    if not platforms:
-        platforms = []
+    if platforms:
+        platforms = frozenset(platforms)
+    else:
+        platforms = frozenset()

chartpress/chartpress.py

Lines 339 to 340 in d625f46

if not platforms:
platforms = []


From some code inspection, this seems viable to do. Is that a sensible change? it would enable us to have a single function instead of two separate, and still wouldn't be a breaking API change for build_images.

@consideRatio
Copy link
Member

@manics this looks good, and it successfully opted to not push when i tried out the reproduction steps in #132 (comment) (that i had small errors in, but fixed them just now).

If you think its worth the time and effort, and it is a sensible idea to refactor platforms to be a frozenset before the helper functions checking if build/push needs to be done are called, then do that but otherwise I think we should go for a merge!

@manics
Copy link
Member Author

manics commented Jul 26, 2021

Good point, done!

@consideRatio
Copy link
Member

@manics wieeee!!! Thanks for your work on multi platform stuff, I love it!!

@consideRatio consideRatio merged commit c4ac075 into jupyterhub:main Jul 26, 2021
@consideRatio consideRatio added the enhancement New feature or request label Jul 26, 2021
@consideRatio consideRatio changed the title _image_needs_pushing: check platforms when using docker buildx _image_needs_[building|pushing]: check platforms when using docker buildx Jul 26, 2021
@manics manics deleted the image_needs_pushing_platforms branch July 26, 2021 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avoid rebuilding an image already available in registry when two --platform flags are passed
2 participants