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
tiltfile: add default_registry(single_name) to push all images to the same image name #3699
Conversation
All the RefSet infrastructure made this a lot easier!!! |
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.
lgtm (except for the part where i have no idea how pushing all images to the same image name works 🤔 )
if defaultReg == "" { | ||
return rs.AsNamedOnly(), nil | ||
} | ||
|
||
// validate the ref produced | ||
newNs := fmt.Sprintf("%s/%s", defaultReg, escapeName(rs.RefFamiliarName())) | ||
newNs := "" |
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 wasn't your variable name but while you're here--this looks like newNamespace
and took me a sec to process, maybe rename?
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 also wondered about this! fixed
internal/container/reference.go
Outdated
@@ -108,7 +109,12 @@ func (rs RefSet) ClusterRef() reference.Named { | |||
} | |||
|
|||
// TagRefs tags both of the references used for build/deploy with the given tag. |
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.
new func name in docstring
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.
done
func (rs RefSet) TagRefs(tag string) (TaggedRefs, error) { | ||
func (rs RefSet) AddTagSuffix(suffix string) (TaggedRefs, error) { | ||
tag := suffix | ||
if rs.registry.SingleName != "" { |
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.
comment about the ECS special case?
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.
added it to the function comment
// https://aws_account_id.dkr.ecr.region.amazonaws.com/my-repo | ||
// (They call this a repo). | ||
// | ||
// For this reason, some users using ECR prefer to push all images to a single image name. |
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... what?? how does this even work?!
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 it a bit locally with tilt.build sending all the images to
default_registry('localhost:3000', single_name='dev-repo')
you end up creating images like
localhost:3000/dev-repo:tilt-site-tilt-deadbeef
let me add a link to docs on this.
i think this is definitely in the "minimum viable solution" space (it's a bit simpler than the original "image transform function" solution suggested). I am curious how people will end up using it!
… same image name Fixes #2419
Hello @maiamcc,
Please review the following commits I made in branch nicks/ch8726:
c8e1b3e (2020-08-12 19:50:01 -0400)
tiltfile: add default_registry(single_name) to push all images to the same image name
Fixes #2419
0422a04 (2020-08-12 19:35:49 -0400)
tiltfile: add a stringable type to make it easier to unpack strings
Code review reminders, by giving a LGTM you attest that: