-
Notifications
You must be signed in to change notification settings - Fork 327
feat: pass docker arch to ecr to lambda #3046
Conversation
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 know this is still in draft, but figured I'd give a lil' review!
builtin/docker/plugin.proto
Outdated
|
||
// Waypoint 0.6 and earlier supported "img" as a Dockerless builder. | ||
// We removed this in 0.7, hence we prefix this now with "unused". We | ||
// keep the field around so we can detect old clients sending it and | ||
// show an error. Do not set this. | ||
google.protobuf.Empty unused_img = 5; | ||
google.protobuf.Empty unused_img = 6; |
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.
if I remember right, these numbers should never be changed. Otherwise old proto clients will use the wrong variables during communication. It's totally fine to make architecture
the next number here, no need to bump the other ones.
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.
Ah TIL! and that makes sense
builtin/aws/lambda/mapper.go
Outdated
log.Warn("unsupported docker architecture", "arch", src, "defaulting to:", lambda.ArchitectureX8664) | ||
return lambda.ArchitectureX8664 |
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.
This could just be the default
case in the switch
59108a2
to
59249dd
Compare
c89f0db
to
749d1da
Compare
1787958
to
d007daa
Compare
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.
If you change that file so it's not confused as an argmapper mapper, we'll be good to go!
builtin/aws/lambda/mapper.go
Outdated
@@ -0,0 +1,19 @@ | |||
package lambda |
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.
Could you move this to utils.go
instead? Mappers are a specific argmapper concept that are automatically called and have to be registered. This appears to just be a helper function to convert one string to another.
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.
TIL! Ok, renamed this mapper.go
file utils.go
Description
This adds
docker
architecture, and passes it to theecr
component and thenlambda
component (via the protobufs) for a 0-config option to automatically set a lambda architecture that is compatible with the ECR image architecture.The HCL value from #3032 takes precedence.
This closes #3026