Skip to content
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

Option to re-enable docker build on preview #540

Closed
NakulK48 opened this issue Mar 13, 2023 · 9 comments · Fixed by #855
Closed

Option to re-enable docker build on preview #540

NakulK48 opened this issue Mar 13, 2023 · 9 comments · Fixed by #855
Assignees
Labels
customer/feedback Feedback from customers kind/enhancement Improvements or new features resolution/fixed This issue was fixed

Comments

@NakulK48
Copy link

NakulK48 commented Mar 13, 2023

Hello!

  • Vote on this issue by adding a 👍 reaction
  • If you want to implement this feature, comment to let us know (we'll work with you on design, scheduling, etc.)

Issue details

I noted in the v4.0.0 changelog that running pulumi preview on a stack containing a docker.Image will no longer build the image https://github.com/pulumi/pulumi-docker/releases/tag/v4.0.0

This is advertised as a feature, but we actually find it quite useful to have a verification of the correctness of a dockerfile before applying the update (e.g. to run in CI on every commit in a PR). Is it possible to have a flag that re-enables preview building, or is there a better approach to validating the dockerfile pre-release?

Affected area/feature

Something like:

const image = new docker.Image(deploymentName, {
  build: {
    context: "..",
    dockerfile: "../docker/Dockerfile",
  },
  imageName: repo.repositoryUrl,
  localImageName: deploymentName,
  buildOnPreview: true
});
@NakulK48 NakulK48 added kind/enhancement Improvements or new features needs-triage Needs attention from the triage team labels Mar 13, 2023
@AaronFriel
Copy link
Contributor

@NakulK48 Thanks for flagging this as a feature request! Could you add an example of how a user would use this flag to your issue?

@AaronFriel AaronFriel removed the needs-triage Needs attention from the triage team label Mar 13, 2023
@jamie1911
Copy link

jamie1911 commented Mar 13, 2023

+1 for this.

As someone who approves the PRs developers and other members send my way, I would like to see the successful build process before approving the PR.

We use automation that does the pulumi preview in the PR and we check the pipelines to see what changes/etc have been made and everything runs successfully, before approving the PR.

Now in its >= 4.0.0 state, someone can add a bug into the Dockerfile, someone will approve the PR, which succeeded via pulumi preview, and now once it gets into pulumi up, the docker build fails and the entire pipeline is stopped.

In terms of speed up, we use docker cache on the hosts that run the pulumi previews, so a pulumi preview will generate the docker cache for the first time, when pulumi up runs, it uses the docker cache to generate the final image to push. Negating the speed issue anyway, as the actual build must happen once anyway.

@joewragg
Copy link

joewragg commented Apr 3, 2023

+1 for this

@mnlumi mnlumi added the customer/feedback Feedback from customers label Apr 7, 2023
@pgavlin
Copy link
Member

pgavlin commented May 19, 2023

We find this useful internally for the same reason as called out above:

we actually find it quite useful to have a verification of the correctness of a dockerfile before applying the update

@AaronFriel
Copy link
Contributor

What behavior would you expect if you passed this flag and any other property was unknown. This would occur if it was an Output that hadn't been resolved at preview time.

Do we:

  1. Skip the build despite the flag, silently continue.

  2. Fail with an error

  3. Try the build with the option ignored

@AaronFriel
Copy link
Contributor

AaronFriel commented Jun 1, 2023

What behavior would you expect if you passed this flag and any other property was unknown. This would occur if it was an Output that hadn't been resolved at preview time.

Do we:

  1. Skip the build

  2. Fail with an error

  3. Try the build without the unknown values (e.g.: missing a build arg, or missing another input to the Image)

@pgavlin
Copy link
Member

pgavlin commented Jun 1, 2023

I’d say skip the build and issue a status log message to notify the user that the build was skipped.

@jamie1911
Copy link

Any additional thoughts or current status on this feature? I'd really like to start using some of the newer features of the docker 4.0+ provider.

@stakauskas
Copy link

Commenting here as well. Would be great to have it as a switch. Used it to separate build / deploy stages, as in: build images on preview and only reference images on deploy. This way ensuring no time is wasted during deploy on image builds and ensuring that there will be no build failures on deploy.

guineveresaenger added a commit that referenced this issue Dec 5, 2023
This pull request's intent is to allow a user to build their image
during `pulumi preview` by using the new `buildOnPreview` resource
field.

This field is added to the Image schema as a boolean and defaults to
false.

The implementation specifics are as follows, with some things for the
reviewer to weigh in on.
1. As a prerequisite, `supportPreview` is enabled and all implemented
RPD methods should handle Unknowns. This should also help address #847
and #620.
2. `ContainsUnknowns()` checks are added to the marshaler and some of
the `Check()` logic. I wasn't sure if `ContainsUnknowns()` or
`IsComputed()`should be used here; the former contains a check for the
latter.
3. Unit tests verify the new marshaling behavior.
4. When a Dockerfile is Unknown, we do not verify its location during
Preview `Check()`, instead we apply other defaults and carry on. We will
calculate the build hash on the `update` call once Unknowns are
computed.
5. When in preview mode, and buildOnPreview is false, we return all
inputs as-is in Update() and Create().
6. When an attempt is made to build on preview, but there are Unknowns
in the inputs or news, we send an error instructing the user to set
`buildOnPreview` to false.
7. An integration test is added that verifies an image builds on preview
if the `buildOnPreview` flag is set to true.
8. An integration test is added that verifies an image _fails_ to build
on preview if there are Unknown inputs and the `buildOnPreview` flag is
set to true

Fixes #540.

- Handle unknowns in Build object
- Handle unknowns in Check; skip dockerfile location finding.
- Set SupportPreview to true
- Add ContainsUnknowns() checks for build: target, stages, platform,
Dockerfile, Context; and registry: username, password, server
- Add tests for Unknowns, and tweak Unknown checks as a result of a bit
of TDD
- Add logic to imageBuild that allows for buildOnPreview
- Use Command.stdout to test unknowns
- Add a few more Unknown checks in Check()
- Add an integration test for Build On Preview
- Build SDKs

---------

Co-authored-by: Bryce Lampe <[email protected]>
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer/feedback Feedback from customers kind/enhancement Improvements or new features resolution/fixed This issue was fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants