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

feat: Create multi-arch images #49

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

Conversation

tgtajuckel
Copy link

This is an initial straw man implementation. I'm expecting to clean up source, testing, and commit messages before creating a final PR for merging. The goal for this draft PR of early work is merely to solicit feedback. Some bits remain awkward. The way the plugin is deciding whether to execute Build or Manifest commands is a total kludge, for instance.

Specific questions I'd appreciate feedback on:

  • Is the embedded manifest: configuration acceptable? Does anyone have suggestions for improvement?
  • Does anyone have experience providing or consuming --os-features flags in a list manifest? The docs list it as an array of strings, but I'm not sure if a simple comma-separated list is what is expected. Perhaps I should provide multiple --os-features arguments? I've just honestly not encountered it.
  • Would it be better to move move this work to a separate vela-docker-manifest plugin? I'm hesitant to do so, as the entire repository infrastructure is shared, but the current build/manifest dichotomy feels very awkward.

This is an attempt to resolve go-vela/community#482

@kneal
Copy link
Contributor

kneal commented Jan 31, 2022

Would it be better to move move this work to a separate vela-docker-manifest plugin? I'm hesitant to do so, as the entire repository infrastructure is shared, but the current build/manifest dichotomy feels very awkward.

I think it makes sense to do this work here to keep the plugin consolidated. We have a lot of plugins that use a multi command structure for binaries.

@codecov
Copy link

codecov bot commented Jan 31, 2022

Codecov Report

Merging #49 (251bf3c) into master (d9656b7) will decrease coverage by 8.80%.
The diff coverage is 28.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #49      +/-   ##
==========================================
- Coverage   62.89%   54.08%   -8.81%     
==========================================
  Files           7        8       +1     
  Lines         671      832     +161     
==========================================
+ Hits          422      450      +28     
- Misses        241      367     +126     
- Partials        8       15       +7     
Impacted Files Coverage Δ
cmd/vela-docker/main.go 0.00% <0.00%> (ø)
cmd/vela-docker/plugin.go 17.44% <13.63%> (-0.31%) ⬇️
cmd/vela-docker/manifest.go 32.00% <32.00%> (ø)
cmd/vela-docker/build.go 86.28% <100.00%> (-9.63%) ⬇️

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.

vela should be able to create multi-arch images
2 participants