engine: dcbad can build images [ch1371]#1048
Conversation
| f.WriteFile("missing.txt", "missing") | ||
|
|
||
| ref, err := f.b.BuildDockerfile(f.ctx, f.ps, f.getNameFromTest(), df, f.Path(), model.EmptyMatcher, model.DockerBuildArgs{}) | ||
| ref, err := f.b.BuildDockerfile(f.ctx, f.ps, f.getNameFromTest(), df, f.Path(), model.EmptyMatcher, model.DockerBuildArgs{}, false) |
There was a problem hiding this comment.
have you considered having a separate Tag() call?
i.e., docker tag fortune:tilt-cabcab fortune:latest
(i also don't think TagImage should be part of the normal BuildDockerfile call, but we can make that decision independently)
| tag, err := digestAsTag(dig) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "TagImage") | ||
| func (d *dockerImageBuilder) TagImage(ctx context.Context, ref reference.Named, dig digest.Digest, tagLatest bool) (reference.NamedTagged, error) { |
There was a problem hiding this comment.
maybe there should be two separate methods?
TagImage(reference.NamedTagged, dig Digest)
TagImageFromDigest(reference.Named, dig Digest)
presumably, the second one could use the first one
There was a problem hiding this comment.
no longer relevant after refactor
| case model.ImageTarget: | ||
| iTargets = append(iTargets, s) | ||
| case model.DockerComposeTarget: | ||
| dcTargets = append(dcTargets, s) |
There was a problem hiding this comment.
should probably return nil if there's a spec it doesn't recognize, rather than quietly dropping it on the floor. see the other extract() method
| return store.BuildResultSet{}, err | ||
| } | ||
| if haveImage { | ||
| logger.Get(ctx).Infof("~~ i'm building ur image for: %s", dcTarget.Name) |
| // NOTE(maia): we assume that this func takes one DC target and up to one image target | ||
| // corresponding to that service. If this func ever supports specs for more than one | ||
| // service at once, we'll have to match up image build results to DC target by ref. | ||
| ref, err = bd.ibd.build(ctx, iTarget, currentState[iTarget.ID()], ps, true) |
There was a problem hiding this comment.
maybe this is a good sign that this needs to be broken out into a common utility class? it seems very weird, architecturally, for one ImageBuildAndDeployer to container another.
f7cd64a to
f719591
Compare
… build-and-deployers can share
| } | ||
| } | ||
|
|
||
| func (icb *ImageAndCacheBuilder) Build(ctx context.Context, iTarget model.ImageTarget, state store.BuildState, ps *PipelineState, canSkipPush bool) (reference.NamedTagged, error) { |
There was a problem hiding this comment.
should canSkipPush be a method of this struct? it seems like a weird thing to be a parameter
There was a problem hiding this comment.
yeahhh we'd need to mess with the way we k8s env in and how it interacts with Docker Compose -- because we still populate this value even when we're doing Docker Compose stuff instead of worrying about k8s :-/ i'll leave a todo
| } else { | ||
| // !shouldBuild implies that Tilt will take care of building, which implies that | ||
| // we should recreate container so that we pull the new image | ||
| // NOTE(maia): this is maybe the WRONG thing to do if we're deploying a service |
There was a problem hiding this comment.
for what it's worth, this is another reason why our k8s-based builder uses content-based tags: it's a way to make sure kubernetes creates a new pod iff the contents changed.
| @@ -0,0 +1,175 @@ | |||
| package build | |||
There was a problem hiding this comment.
fwiw, i probably would have left this in the same package...i feel like moving it to a different package forced you to make a bunch of odd architecture decisions around mode, which distracts from the main focus of this PR
a580fda to
571c690
Compare
2 participants
tests to follow but want a check first that i'm on a reasonable track