-
Notifications
You must be signed in to change notification settings - Fork 404
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
Multi-platform ko #38
Conversation
google/go-containerregistry#474 would make my proposed implementation easier. |
pkg/build/gobuild.go
Outdated
return nil, fmt.Errorf("oops") | ||
} | ||
|
||
type index struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of this could go away with google/go-containerregistry#474
Could there be a flag or environment variable to customize this behavior ? (like it does what you describe by default but the user could force to build a singleton manifest list) |
Hm.. that gives me an idea. We could add a |
I like that idea 😻
on the |
@jonjohnsonjr If |
Agreed, my first thought was to just default to amd64/linux (since MacOS -> Linux Kubernetes is so common), and it's the default in docker distribution. Using GOARCH/GOOS also makes sense. Another idea I just had was to try to derive the os/arch based on the cluster we're deploying to:
Though, that would add a little bit of latency to everything... Some combination of those things is probably going to be the best user experience, e.g.:
|
That's great, make sense, expect to see the new implements |
Hi @jonjohnsonjr , @junawaneshivani and I have been working on testing the changes in this PR on ppc64le platform. Error encountered:
Are we missing any step? |
@lysannef |
@vincent-pli These are the steps I took:
trying a go build in this repository, still throws the same error.
Could you please elaborate on the same. We are trying to test the changes made in @jonjohnsonjr 's repository at the multi-platform-wip branch. How would changing to the original ko repo work? |
So what's the path of your |
@vincent-pli The path we are testing: github.com/jonjohnsonjr/ko/cmd/ko Using the path suggested by you : github.com/google/ko
|
I'm little confuse, so I just try it on ppc64le
It works, and get correct result. |
I followed these steps and this is the result I get:
Based on the ouput given above I dont see the additional images being created , like
Yes I am an IBMer |
@lysannef you can find me by ST: Peng QL Li 😄 |
@jonjohnsonjr We have validated the working of these changes on ppc64le architecture. The architecture specific images are generated , and the manifest is as below.
Have there been any plans about up-streaming these changes and around when would that be expected? |
Pinging back here for a quick update. I just landed google/go-containerregistry#520 which I believe is everything I need upstream to implement this "for real". I'll try to revisit this PR in the next couple weeks. @mattmoor had a nice idea that the default flag value for platforms should depend on which command we're invoking:
|
Hi @jonjohnsonjr, any further progress on this PR? |
da7c5e9
to
2a797ec
Compare
Just pushed something that I'm pretty happy with. I'm not 100% certain what we want to do with the |
62a7be5
to
532aa42
Compare
532aa42
to
3ea73f0
Compare
@jonjohnsonjr I see there have been few commits since I last checked.. Does the |
Hi @jonjohnsonjr |
@jonjohnsonjr just wanted to check is there is any work remaining in terms of validating these changes with which we @junawaneshivani and myself on the Power side can help with? or any other blockers that need to be worked through? thanks in advance! |
026422b
to
03a8e82
Compare
Generally, this seems like a good first chunk to land, but we should open issues for "smarter" platform modes. Honestly, looking through, I'm pretty happy with how simple this was. Most of the diff seemed like changing signatures, which is pretty cool. I'd like to get an early form of this in, land #162 and then cut 0.6 🤩 |
Oh, one other thing I've been thinking is that when the base image is an image index, it would be good for use to log what platforms we see and build for logged to stderr. |
I did this really lazily by just logging architecture every time in our |
I saw, seemed fine. |
1b78f95
to
26015f8
Compare
🎉 |
This is a sketch of what I had in mind for #6
For this PR, I've set the default base image for this repo to
ubuntu
so that you can just fetch this branch and runko publish ./cmd/ko
to test out the change. It should build a different binary for every platform and publish a manifest list instead of just a manifest.I haven't actually tested pulling/running this on different platforms (I don't have any), so YMMV, but publishing works 👍
I don't actually like this very much because I've made every package deal with a new abstraction (I've called it
Steve
so that we don't ever merge this) that can represent either base image or a base manifest list. A better approach would probably be to make everything deal with a manifest list to simplify things.When pulling base image:
When publishing the artifacts:
This isn't perfect (what if the user actually does want to publish a singleton manifest list?), so maybe it's not the best idea. Open to suggestions :)