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

Message sooner about building for the host architecture #847

Open
cnunciato opened this issue Nov 15, 2023 · 3 comments
Open

Message sooner about building for the host architecture #847

cnunciato opened this issue Nov 15, 2023 · 3 comments
Labels
kind/enhancement Improvements or new features

Comments

@cnunciato
Copy link

The message we currently show about building the image for the host architecture only appears at the end of an update. We should instead show this message during the preview so that users can correct the problem before running an update (and often waiting several minutes for health checks, etc., to time out) that ultimately fails.

@cnunciato cnunciato changed the title Message sooner about non- Message sooner about building for the host architecture Nov 15, 2023
@AaronFriel
Copy link
Contributor

AaronFriel commented Nov 15, 2023

This may be a duplicate of #620, or it may rely on other unknowns being present in the build object. Does this repro on the latest Docker Image?

Locally:

$ pulumi preview
...
  docker:index:Image (ai):
    Building your image for linux/amd64 architecture.
    To ensure you are building for the correct platform, consider explicitly setting the `platform` field on ImageBuildOptions.

@cnunciato
Copy link
Author

cnunciato commented Nov 15, 2023

Using awsx.ecr.Image, I do not see that:

image

I do see it for docker.Image, however. Should we be logging this somehow differently such that it's able to be picked up by dependents?

@AaronFriel
Copy link
Contributor

AaronFriel commented Nov 15, 2023

Ok - the cause is the same as #620, but likely not fixed by the same PR.

@mikhailshilkov mikhailshilkov added the kind/enhancement Improvements or new features label Nov 16, 2023
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Improvements or new features
Projects
None yet
Development

No branches or pull requests

3 participants