-
Notifications
You must be signed in to change notification settings - Fork 326
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Looks good, just one minor thing to work around the ECR architecture issue you pointed out.
builtin/aws/ecr/pull/builder.go
Outdated
// TODO(kevinwang): Do we need to get architecture? | ||
// If we do, we can pull the image and inspect it via `cli.ImageInspectWithRaw`, | ||
// - prior art: /builtin/docker/builder.go -> Build | ||
// There is also an open issue for the ECR team to build a architecture feature into | ||
// the UI, which probably comes with a CLI/API change. | ||
// - see https://github.com/aws/containers-roadmap/issues/1591 |
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.
Since ECR doesn't expose the architecture in an obvious way here yet, I'd probably make an option on the plugin called force_architecture
that will set the architecture on output. This will make it at least work with the other bits we've been adding architecture to.
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.
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.
Looks great!
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.
I tested this out with a local & remote runner, looks great! I've also added an example usage of this plugin to the examples repo.
Resolves #3390
Checklist
Usage