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.

Sign up for GitHub

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

Jump to bottom

Add --target to bud command #1321

Closed
wants to merge 1 commit into from
Closed

Add --target to bud command #1321

wants to merge 1 commit into from

Conversation

TomSweeneyRedHat
Copy link
Member

@TomSweeneyRedHat TomSweeneyRedHat commented Feb 3, 2019 •
edited
Loading

Signed-off-by: TomSweeneyRedHat [email protected]

Add the --target option to the bud command. This allows the
user to specify the last stage to build in a multi stage
Dockerfile.

Addresses #632

Tests:

cat ./bud/target/Dockerfile
FROM ubuntu:latest
RUN touch /1

FROM alpine:latest AS mytarget
RUN touch /2

FROM busybox:latest AS mytarget2
RUN touch /3

buildah bud --debug=false -t tom --target mytarget ./bud/target .
STEP 1: FROM ubuntu:latest
STEP 2: RUN touch /1
STEP 3: FROM alpine:latest AS mytarget
STEP 4: RUN touch /2
STEP 5: COMMIT containers-storage:[overlay@/var/lib/containers/storage+/var/run/containers/storage]localhost/tom:latest
Getting image source signatures
Skipping blob 503e53e365f3 (already present): 5.52 MiB / 5.52 MiB [=========] 0s
Copying blob 66e5f29a7649: 3.50 KiB / 3.50 KiB [============================] 0s
Copying config e72f1fa3d72d: 704 B / 704 B [================================] 0s
Writing manifest to image destination
Storing signatures
--> e72f1fa3d72ddd3f23acb22a059ecce33dad571433223389e3ce92a5fd9ebae5
STEP 6: COMMIT containers-storage:[overlay@/var/lib/containers/storage+/var/run/containers/storage]localhost/tom:latest
Getting image source signatures
Skipping blob 503e53e365f3 (already present): 5.52 MiB / 5.52 MiB [=========] 0s
Skipping blob 66e5f29a7649 (already present): 3.50 KiB / 3.50 KiB [=========] 0s
Copying config e72620d8efe7: 704 B / 704 B [================================] 0s
Writing manifest to image destination
Storing signatures
--> e72620d8efe764178d1352dfb3a9a773794309ee9e879e17d3803b18553f5025

buildah images
IMAGE NAME                                               IMAGE TAG            IMAGE ID             CREATED AT             SIZE
docker.io/library/ubuntu                                 latest               20bb25d32758         Jan 22, 2019 17:41     90 MB
docker.io/library/alpine                                 latest               caf27325b298         Jan 30, 2019 17:19     5.8 MB
localhost/tom                                            latest               993ee7ded616         Feb 5, 2019 13:49      5.8 MB

Signed-off-by: TomSweeneyRedHat [email protected]

@TomSweeneyRedHat
Copy link
Member Author

@mheon FYI. I'll need to touch up the man page on Podman and vendor Buildah once this rumbles through.

@mheon
Copy link
Member

mheon commented Feb 3, 2019

SGTM

rhatdan
rhatdan reviewed Feb 3, 2019
View reviewed changes
imagebuildah/build.go Outdated
if (options.Target != "") {
stagesTargeted, ok := stages.ByTarget(options.Target)
if !ok {
return "", nil, errors.Wrapf(err, "The target %q was not found in the provided Dockerfile", options.Target)
Copy link
Member

Choose a reason for hiding this comment

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

Where did the err come from? I think this is a bug. Should just be a errors.Errorf()

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, cut/paste error from imagebuilder, I'll touch it up.

@pixdrift pixdrift mentioned this pull request Feb 4, 2019
Closed
@TomSweeneyRedHat TomSweeneyRedHat force-pushed the dev/tsweeney/target branch 2 times, most recently from 20d8a32 to 984bbaa Compare February 4, 2019 22:30
@TomSweeneyRedHat
Copy link
Member Author

Repushed with gofmt tended to and @rhatdan's comment addressed.

@rhatdan
Copy link
Member

rhatdan commented Feb 4, 2019

LGTM
@umohnani8 @mheon @vrothberg @giuseppe @pixdrift PTAL

umohnani8
umohnani8 reviewed Feb 5, 2019
View reviewed changes
tests/bud/target/Dockerfile
FROM fedora:latest
RUN touch /foo

FROM alpine:latest AS mytarget
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add one more stage after this to ensure that it is skipped when --target it set to mytarget. To ensure we don't accidentally regress in future code changes.

@umohnani8
Copy link
Member

One comment, but LGTM

@vrothberg
Copy link
Member

Moby seems to behave differently. Currently, we only execute the target but Moby is stopping at the specified target and executes all previous stages.

FROM alpine:latest as one        
RUN touch ~/alpine.txt           
                                 
FROM centos:latest as centos     
RUN touch ~/centos.txt           
                                 
FROM fedora:latest as fedora     
RUN touch ~/fedora.txt

Running docker build --target centos on the upper Dockerfile:

Sending build context to Docker daemon  2.048kB
Step 1/4 : FROM alpine:latest as one
latest: Pulling from library/alpine
6c40cc604d8e: Pull complete
Digest: sha256:b3dbf31b77fd99d9c08f780ce6f5282aba076d70a513a8be859d8d3a4d0c92b8      
Status: Downloaded newer image for alpine:latest
 ---> caf27325b298
Step 2/4 : RUN touch ~/alpine.txt
 ---> Running in 923b3b87dfd6
Removing intermediate container 923b3b87dfd6
 ---> 544aae7ed2c4
Step 3/4 : FROM centos:latest as centos
latest: Pulling from library/centos
a02a4930cb5d: Pull complete
Digest: sha256:184e5f35598e333bfa7de10d8fb1cebb5ee4df5bc0f970bf2b1e7c7345136426      
Status: Downloaded newer image for centos:latest
 ---> 1e1148e4cc2c
Step 4/4 : RUN touch ~/centos.txt
 ---> Running in c46c76e02eca
Removing intermediate container c46c76e02eca
 ---> ef6c384fa716
Successfully built ef6c384fa716

@TomSweeneyRedHat TomSweeneyRedHat mentioned this pull request Feb 5, 2019
Merged
@TomSweeneyRedHat TomSweeneyRedHat changed the title Add --target to bud command WIP - Add --target to bud command Feb 5, 2019
@TomSweeneyRedHat
Copy link
Member Author

I've had to change this to WIP until openshift/imagebuilder#121 is merged and then I can vendor it here. Test should fail until that happens as the new ThroughTarget() function is not yet available.

@TomSweeneyRedHat
Add --target to bud command
dd54096 
Signed-off-by: TomSweeneyRedHat <[email protected]>

Add the --target option to the bud command.  This allows the
user to specify the last stage to build in a multi stage
Dockerfile.

Addresses containers#632

Tests:
```
cat ./bud/target/Dockerfile
FROM ubuntu:latest
RUN touch /1

FROM alpine:latest AS mytarget
RUN touch /2

FROM busybox:latest AS mytarget2
RUN touch /3

buildah bud --debug=false -t tom --target mytarget ./bud/target .
STEP 1: FROM ubuntu:latest
STEP 2: RUN touch /1
STEP 3: FROM alpine:latest AS mytarget
STEP 4: RUN touch /2
STEP 5: COMMIT containers-storage:[overlay@/var/lib/containers/storage+/var/run/containers/storage]localhost/tom:latest
Getting image source signatures
Skipping blob 503e53e365f3 (already present): 5.52 MiB / 5.52 MiB [=========] 0s
Copying blob 66e5f29a7649: 3.50 KiB / 3.50 KiB [============================] 0s
Copying config e72f1fa3d72d: 704 B / 704 B [================================] 0s
Writing manifest to image destination
Storing signatures
--> e72f1fa3d72ddd3f23acb22a059ecce33dad571433223389e3ce92a5fd9ebae5
STEP 6: COMMIT containers-storage:[overlay@/var/lib/containers/storage+/var/run/containers/storage]localhost/tom:latest
Getting image source signatures
Skipping blob 503e53e365f3 (already present): 5.52 MiB / 5.52 MiB [=========] 0s
Skipping blob 66e5f29a7649 (already present): 3.50 KiB / 3.50 KiB [=========] 0s
Copying config e72620d8efe7: 704 B / 704 B [================================] 0s
Writing manifest to image destination
Storing signatures
--> e72620d8efe764178d1352dfb3a9a773794309ee9e879e17d3803b18553f5025

buildah images
IMAGE NAME                                               IMAGE TAG            IMAGE ID             CREATED AT             SIZE
docker.io/library/ubuntu                                 latest               20bb25d32758         Jan 22, 2019 17:41     90 MB
docker.io/library/alpine                                 latest               caf27325b298         Jan 30, 2019 17:19     5.8 MB
localhost/tom                                            latest               993ee7ded616         Feb 5, 2019 13:49      5.8 MB
```

Signed-off-by: TomSweeneyRedHat <[email protected]>
@TomSweeneyRedHat
Copy link
Member Author

Since the last push, I've updated the vendor for imagebuilder and made one small change in wording in the man page. Fingers crossed, this is gtg once the tests pass.

@TomSweeneyRedHat TomSweeneyRedHat changed the title WIP - Add --target to bud command Add --target to bud command Feb 12, 2019
@rhatdan
Copy link
Member

rhatdan commented Feb 13, 2019

I will leave this to @vrothberg to approve.

vrothberg
vrothberg reviewed Feb 13, 2019
View reviewed changes
Copy link
Member

@vrothberg vrothberg 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 @TomSweeneyRedHat!

@vrothberg
Copy link
Member

@rh-atomic-bot r+

@rh-atomic-bot
Copy link
Collaborator

📌 Commit dd54096 has been approved by vrothberg

@rh-atomic-bot
Copy link
Collaborator

⚡ Test exempted: pull fully rebased and already tested.

@TomSweeneyRedHat TomSweeneyRedHat deleted the dev/tsweeney/target branch February 18, 2019 14:56
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@umohnani8 umohnani8 umohnani8 left review comments

@vrothberg vrothberg vrothberg left review comments

@rhatdan rhatdan rhatdan left review comments

Assignees
No one assigned
Labels
locked - please file new issue/PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@TomSweeneyRedHat @mheon @rhatdan @umohnani8 @vrothberg @rh-atomic-bot