Perform docker builds even during preview so we can get users errors prior to a real update. #135
Conversation
CyrusNajmabadi
changed the title
| }, | ||
| Dir: path.Join(cwd, "broken_dockerfile"), | ||
| ExpectFailure: true, | ||
| SkipRefresh: true, |
There was a problem hiding this comment.
note: i will be able to pass in SkipUpdate here once pulumi/pulumi#3894 goes in. This will allow us to validate that it's the preview step that is causing us to fail.
sdk/nodejs/docker.ts
Outdated
|
|
||
| logEphemeral("Starting docker build...", logResource); | ||
| const buildResult = await buildImage(imageName, pathOrBuild, repositoryUrl, logResource, connectToRegistry); | ||
| logEphemeral("Completed docker build", logResource); |
There was a problem hiding this comment.
we always attempt to do the build.
sdk/nodejs/docker.ts
Outdated
|
|
||
| logEphemeral("Starting docker push...", logResource); | ||
| const result = await pushImage(repositoryUrl, buildResult, logResource); | ||
| logEphemeral("Completed docker build", logResource); |
There was a problem hiding this comment.
but we only attempt to push if we're in a normal update.
|
|
||
| const tag = utils.getImageNameAndTag(baseImageName).tag; |
There was a problem hiding this comment.
we don't need the tag except for pushing. so this moved into pushImageAsync.
| @@ -283,24 +315,24 @@ async function buildAndPushImageWorkerAsync( | |||
|
|
|||
| // Use those to push the image. Then just return the unique target name. as the final result | |||
| // for our caller to use. Only push the image during an update, do not push during a preview. | |||
| if (!pulumi.runtime.isDryRun() && !skipPush) { | |||
There was a problem hiding this comment.
this moved up to the top-level logic.
|
Narrowly, this feels like the right direction - but I am nervous due to the large number of structural issues we have currently with the
And ultimately - how this will align with (or not align with) where we really want to go with #132. Do you have a model for how this change would behave in a world where we were participating in Check/Diff/Create/Update/Delete more explicitly? |
| logEphemeral("Completed docker build", logResource); | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
tried to make it very clear in teh top level function what the overall big-steps are that we're performing. The nuts-and-bolts logic is now moved into the helpers.
| // a value yet for `pathOrBuild`. So we do a normal `ouput(...).apply(...)`. However, we *do* | ||
| // want proceed if we don't have a value yet for `repositoryUrl`. In that case, we'll just | ||
| // build without actually pushing. To support that, we run `.apply` on the repoUrl, but we pass | ||
| // in `runWithUnknowns:true` to actually continue on in that case. |
There was a problem hiding this comment.
note: higher level layers will likely need the same. i.e. we call buildAndPushImage from inside Outputs higher up that will likely not run since they won't have a known repoUrl. Will be making further changes in pulumi/awsx once this is in.
Yes, the way i envision it is that we would likely need to move to a world where we hash local state and use that to determine what would need to happen. Specifically, we would effectively treat teh dockerfile/dir as an asset/archive and drive nearly everything off of that. We would likely need an opt-out flag to allow people to say that the full docker process should run even if everything was hash-identical. However, this should likely be the rare case, as the 'immutable infrastructure' view of things really wants to convey that you should get the same outputs if your inputs don't change. Clearly that doesn't have to be true for docker (since it can absolutely be affected by anything), but we should model the immutable form by default. Given this, we could likely switch to a model where Image truly was a custom resource, with all the logic for it was in basically a dynamic provider. It would appear just as an immutable resource type to anyone using it, with all this logic here used to impl those properties. |
Yes. Looking at those issues, i can immediately see a problem right off the bat. We clearly have gotten feedback that just even doing the pull/build during preview can be problematic as it just takes time that often isn't important given the lack of change anywhere else. Because of that, it would likely make sense even in this PR to allow one to opt out of any of this behavior. i.e. we might change the default to build-during-preview, but we likely should still have a way to say "but i don't want to do that" for cases where the cost is high, and the value is low. i.e. for some users who aren't routinely changing their docker file, just doing this work during preview won't be desirable. We shoudl have a way for them to opt out of this. |
|
Curious what your thoughts are here @joeduffy . You've mentioned both the frustration of having long times waiting for docker, but also the desire to have quick feedback about if there's a problem with a docker build. Perhaps we coudl get a best-of-all-worlds approach here by making this a real custom resource. I thnk (but would like @pgavlin 's thoughts here) that this might just fall-out if we do that. Here's why: If we make this a real custom-resource, then we can cache things like a hash of the dockerfile/dir. Then, we can actually do a true 'diff' in the future automatically stating that nothing changed if your hashes were the same. This would likely lead us to a place where, by default, you only paid for docker on the first preview/update, or when you actually changed something. Otherwise, we would continually return the previously cached values. We might potentially need an opt-out flag/env variable to allow users to say that didn't want us to use those hashes (i.e. because the docker build would produce a new result due to factors outside their environment), but that doesn't seem hard to provide. |
|
I think we should put this PR on pause for now and consider allocating some real amount of time to explore the actual provider route here. I would be happy to start looking into that now. |
|
Docker already does fairly sophisticated caching. Are we sure it doesn't already hash the build context and use that to determine when and what to rebuild? Very possible it only uses the Docker build layers and nothing about the filesystem inputs themselves. |
|
Closing out. We'd like to go with a much richer approach here with a real Docker provider. It's something i'm still interested in producing if I have the time for it :) |
Fixes pulumi/pulumi-awsx#472
Should be reviewed after #137 goes in.