build: custom build can support deterministic tags (rather than generating temp tag to check against) [ch1909]#1343
Conversation
maiamcc
changed the title
|
|
||
| type RefSelector struct { | ||
| ref reference.Named | ||
| Ref reference.Named |
There was a problem hiding this comment.
i'm confident that this shouldn't be a public field. it's not something that an API user would want or be able to use correctly. as a proof of point, the only place you did use this field is wrong and a bug.
There was a problem hiding this comment.
Not a bug, it was desired behavior -- the idea was to pass in the ref as specified in Tiltfile (rather than as stripped down for deployment) so we knew whether user gave it a tag or not. But I'm redoing this PR on advice from DB, so it's moot.
There was a problem hiding this comment.
hmmm...can you point me to the spec that describes the expected behavior and how it's supposed to interact with substitute_repo?
There was a problem hiding this comment.
(my understanding from previous conversations is we expected custom_build to push to a remote repo in some cases)
There was a problem hiding this comment.
that behavior has been removed, so i'm not sure your question still applies...?
| ps.StartPipelineStep(ctx, "Building Dockerfile: [%s]", userFacingRefName) | ||
| defer ps.EndPipelineStep(ctx) | ||
| ref, err := icb.custb.Build(ctx, refToBuild, bd.Command) | ||
| ref, err := icb.custb.Build(ctx, iTarget.ConfigurationRef.Ref, bd.Command) |
There was a problem hiding this comment.
shouldn't this be the DeploymentRef? see other places where we do builds, e.g. https://github.com/windmilleng/tilt/blob/master/internal/engine/image_and_cache_builder.go#L37
|
This is how it looks in action: https://github.com/windmilleng/servantes/tree/bazel-tag-param |
| } | ||
|
|
||
| if strings.Contains(f.k8s.Yaml, sidecar.SyncletImageName) { | ||
| t.Errorf("Should not deploy the synclet for a custom build: %s", f.k8s.Yaml) |
There was a problem hiding this comment.
hmmm...this behavior is surprising to me? is there something i can read that explains why custom_build would affect synclet behavior?
There was a problem hiding this comment.
Huhhh idk, I copied this from another test 🤷♀️ i think it's relevant b/c it's a custom build W/O fast build? (See this test. It's not relevant to this test case tho, I'll take it out.
| t.Errorf("Expected 0 docker build, actual: %d", f.docker.BuildCount) | ||
| } | ||
|
|
||
| if f.docker.PushCount != 1 { |
There was a problem hiding this comment.
hmmm...afaict, this test would pass fine if the functionality in this pr was totally broken...right? please also add an assertion about the image that was pushed
There was a problem hiding this comment.
nope, it fails without the rest of the functionality in this PR. if CustomBuild.Tag isn't set, we generate our own temporary tag and set an env variable $TAG=imagename:temptag; we then check that the expected image and expected tag were built.
if i remove the changes to CustomBuilder.Build, test fails with:
Custom Build: Injecting Environment Variables
TAG=gcr.io/some-project-162817/sancho:tilt-build-1551202573
Running custom build cmd "true"
ERROR: fake docker client error: object not found (fakeClient.Images key: gcr.io/some-project-162817/sancho:tilt-build-1551202573)
(i.e. we spoof that the custom build command built gcr.io/.../sancho:deterministic-tag; but Build goes looking for sancho:tilt-build-1551202573, cant find it, thinks something has gone wrong.)
jazzdan
left a comment
jazzdan
left a comment
There was a problem hiding this comment.
LGTM! Thanks for taking care of this :)
nicks
left a comment
nicks
left a comment
There was a problem hiding this comment.
Great, thx for the explanations!
3 participants
in Tiltfile, user can pass an optional
tagarg tocustom_build. If this arg is set, we execCustomBuild.commandand check that it produceduser-image:customBuild.Tag; otherwise, we generate our own temp tag, exec the command, and check that it produceduser-image:tmpTag.