Skip to content

builder/dockerfile: errmsg: quote build target#46754

Merged
thaJeztah merged 1 commit intomoby:masterfrom
Frankkkkk:master
Nov 2, 2023
Merged

builder/dockerfile: errmsg: quote build target#46754
thaJeztah merged 1 commit intomoby:masterfrom
Frankkkkk:master

Conversation

@Frankkkkk
Copy link
Contributor

@Frankkkkk Frankkkkk commented Nov 1, 2023

Hi,

The build target is not quoted and it makes it difficult for some persons to see what the problem is.

By quoting it we emphasize that the target name is variable.

- Description for the changelog

builder/dockerfile: errmsg: quote build target

- A picture of a cute animal (not mandatory but encouraged)
image

Thanks and cheers !

@Frankkkkk Frankkkkk requested a review from tonistiigi as a code owner November 1, 2023 11:22
AkihiroSuda
AkihiroSuda reviewed Nov 1, 2023
View reviewed changes
@thaJeztah thaJeztah added area/builder Build status/2-code-review kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. area/builder/classic-builder Build labels Nov 1, 2023
@Frankkkkk Frankkkkk force-pushed the master branch 2 times, most recently from e72250a to 1236fd0 Compare November 1, 2023 17:32
@Frankkkkk Frankkkkk mentioned this pull request Nov 1, 2023
Merged
rumpl
rumpl approved these changes Nov 1, 2023
View reviewed changes
Copy link
Member

@rumpl rumpl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

thaJeztah
thaJeztah approved these changes Nov 2, 2023
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!!

@thaJeztah thaJeztah added this to the 25.0.0 milestone Nov 2, 2023
@thaJeztah
Copy link
Member

thaJeztah commented Nov 2, 2023

DOH! Looks like a test needs updating, @Frankkkkk

=== FAIL: github.com/docker/docker/integration-cli TestDockerCLIBuildSuite/TestBuildIntermediateTarget (1.32s)
    docker_cli_build_test.go:5947: assertion failed: 
        Command:  D:\a\moby\moby\out\docker.exe build -t build1 . --target nosuchtarget
        ExitCode: 1
        Error:    exit status 1
        Stdout:   Sending build context to Docker daemon  2.048kB

        
        Stderr:   Error response from daemon: target stage "nosuchtarget" could not be found
        
        
        Failures:
        Expected stderr to contain "failed to reach build target"

=== FAIL: github.com/docker/docker/integration-cli TestDockerCLIBuildSuite (1834.44s)

I see that test is checking for failed to reach build target in the output;

moby/integration-cli/docker_cli_build_test.go

Line 5949 in 60e88c8

Err: "failed to reach build target",

@thaJeztah thaJeztah added status/failing-ci Indicates that the PR in its current state fails the test suite status/2-code-review and removed status/4-merge labels Nov 2, 2023
@Frankkkkk
builder/dockerfile: errmsg: quote build target
123ade3 
The build target is not quoted and it makes it difficult for some
persons to see what the problem is.

By quoting it we emphasize that the target name is variable.

Signed-off-by: Frank Villaro-Dixon <frank.villarodixon@merkle.com>
@thaJeztah thaJeztah added status/4-merge and removed status/2-code-review status/failing-ci Indicates that the PR in its current state fails the test suite labels Nov 2, 2023
thaJeztah
thaJeztah approved these changes Nov 2, 2023
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for updating!

LGTM (assuming CI is happy)

@Frankkkkk
Copy link
Contributor Author

Frankkkkk commented Nov 2, 2023

Hi @thaJeztah I apologize for the lack of oversight.
It should be fixed.
Thanks and have a nice day!

@thaJeztah
Copy link
Member

thaJeztah commented Nov 2, 2023

I apologize for the lack of oversight.

No worries! I also assumed it would pass CI, because it did in the first iteration (that only added the quotes), but we were just lucky because it still was able to match the prefix of the error 😂

@thaJeztah
Copy link
Member

thaJeztah commented Nov 2, 2023

All green now 🎉

@thaJeztah thaJeztah merged commit 217054d into moby:master Nov 2, 2023
This was referenced Jan 22, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda left review comments

@rumpl rumpl rumpl approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

@tonistiigi tonistiigi Awaiting requested review from tonistiigi tonistiigi is a code owner

Assignees

No one assigned

Labels

area/builder/classic-builder Build area/builder Build kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. status/4-merge

Projects

None yet

Milestone

25.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@Frankkkkk @thaJeztah @rumpl @AkihiroSuda