build: add support for `--inherit-labels` #6103

flouthoc · 2025-04-01T13:59:09Z

Allows users to specify if they want to inherit labels from base image or not.

Closes: #6098

What type of PR is this?

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake
/kind other

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

build: allow users to choose if they want to inherit labels from base image or not using --inherit-labels

Honny1 · 2025-04-01T16:51:31Z

/packit retest-failed

packit-as-a-service · 2025-04-01T16:51:38Z

Account Honny1 has no write access nor is author of PR!

nalind

I think this needs to include broader tests, as there are cases I'm not sure are being handled here.

nalind · 2025-04-01T17:50:48Z

define/build.go

@@ -236,6 +236,9 @@ type BuildOptions struct {
 	// ID mapping options to use if we're setting up our own user namespace
 	// when handling RUN instructions.
 	IDMappingOptions *IDMappingOptions
+	// InheritLabels allows users to specify if they want
+	// to inheritlabels from base image or not.


Suggested change

// to inheritlabels from base image or not.

// to inherit labels from base image or not.

nalind · 2025-04-01T17:50:52Z

docs/buildah-build.1.md

@@ -497,6 +497,10 @@ Path to an alternative .containerignore (.dockerignore) file.
 Write the built image's ID to the file.  When `--platform` is specified more
 than once, attempting to use this option will trigger an error.

+**--inherit-labels** *bool-value*
+
+Allows users to specify if they want to inherit labels from base image or not. (Default is `true`).


"Allows users" is kind of implied here, in the man page. Describe what the tool will do when the flag is specified.

Perhaps

Inherit the labels from the base image (default true).

I'm assuming the default is true; I haven't read the rest.

nalind · 2025-04-01T17:56:00Z

tests/bud.bats

+  # and `hello=world` which we just added using cli flag
+  want_output='map["hello":"world" "io.buildah.version":"'$buildah_version'"]'
+  run_buildah inspect --format '{{printf "%q" .Docker.Config.Labels}}' exp
+  expect_output "$want_output"


This needs to test builds with multiple layers (--layers=true) and ensure that the build cache doesn't use images that have been built with one value for this setting when the build is running with a different value, since the resulting configurations would differ.
It should probably also check running a multi-stage build where an earlier stage that's used as the base of a later stage sets a label, as I suspect that might not behave as expected in this iteration.

It would also be good to add a test just using --inherit-labels" to ensure the default works and doesn't get change in the future. I'm also a bit on the OCD side, I'd also like a --inherit-labels=true` test, but not completely necessary.

I added tests to work with --layers

TomSweeneyRedHat · 2025-04-01T21:24:06Z

pkg/cli/common.go

@@ -230,6 +231,7 @@ func GetBudFlags(flags *BudResults) pflag.FlagSet {
 	fs.StringVar(&flags.CertDir, "cert-dir", "", "use certificates at the specified path to access the registry")
 	fs.BoolVar(&flags.Compress, "compress", false, "this is a legacy option, which has no effect on the image")
 	fs.BoolVar(&flags.CompatVolumes, "compat-volumes", false, "preserve the contents of VOLUMEs during RUN instructions")
+	fs.BoolVar(&flags.InheritLabels, "inherit-labels", true, "inherit labels from base image")


Suggested change

fs.BoolVar(&flags.InheritLabels, "inherit-labels", true, "inherit labels from base image")

fs.BoolVar(&flags.InheritLabels, "inherit-labels", true, "inherit the labels from the base image")

lsm5 · 2025-04-02T09:55:28Z

/packit retest-failed

nalind

Tests should also check running a multi-stage build where an earlier stage that's used as the base of a later stage sets a label, as I suspect that might not behave as expected in this iteration, depending on what the expected behavior is, which is unclear.

nalind · 2025-04-09T20:19:07Z

define/build.go

@@ -236,6 +236,9 @@ type BuildOptions struct {
 	// ID mapping options to use if we're setting up our own user namespace
 	// when handling RUN instructions.
 	IDMappingOptions *IDMappingOptions
+	// InheritLabels allows users to specify if they want


Same comment as for the man page: we're at the API level here, so just describe what the library will be doing differently based on this field, perhaps something like "InheritLabels controls whether or not built images will retain the labels which were set in their base images".

tests/bud.bats

nalind · 2025-04-09T20:27:17Z

tests/bud.bats

+  # Now build same file with  --inherit-labels=true and verify if using the cache
+  run_buildah build $WITH_POLICY_JSON --layers --inherit-labels=true -t exp -f $BUDFILES/base-with-labels/Containerfile.layer
+  # Should contain `Using cache` at all since
+  expect_output --substring " Using cache"


How many times is "Using cache" expected to be printed? Would it be simpler to compare the IDs of the final images?

I also added a check below to verify image id.

This should also be checked for multi-stage builds, since for those we generally leave the final image for a stage around for use in caching.

flouthoc · 2025-04-10T18:34:43Z

Tests should also check running a multi-stage build where an earlier stage that's used as the base of a later stage sets a label, as I suspect that might not behave as expected in this iteration, depending on what the expected behavior is, which is unclear.

With this current patch it if --inherit-labels=false is set it does not inherit label into target stage from base stages. I think that aligns with the docs that no labels are inherited at all from base image. I added a test to verify that. WDYT ?

nalind

With this current patch it if --inherit-labels=false is set it does not inherit label into target stage from base stages. I think that aligns with the docs that no labels are inherited at all from base image. I added a test to verify that. WDYT ?

You could ask in the issue that prompted this PR, and given how #5762 describes handling of ARGs, which don't even get recorded in images, I'm not sure if I know what the correct behavior should be for labels when the base image for a stage is part of the same build. Either way, the detail should be mentioned in the docs.

tests/bud.bats

nalind · 2025-04-10T18:47:32Z

tests/bud.bats

+  # Now build same file with  --inherit-labels=true and verify if using the cache
+  run_buildah build $WITH_POLICY_JSON --layers --inherit-labels=true -t exp -f $BUDFILES/base-with-labels/Containerfile.layer
+  # Should contain `Using cache` at all since
+  expect_output --substring " Using cache"


This should also be checked for multi-stage builds, since for those we generally leave the final image for a stage around for use in caching.

flouthoc · 2025-04-14T14:24:07Z

With this current patch it if --inherit-labels=false is set it does not inherit label into target stage from base stages. I think that aligns with the docs that no labels are inherited at all from base image. I added a test to verify that. WDYT ?

You could ask in the issue that prompted this PR, and given how #5762 describes handling of ARGs, which don't even get recorded in images, I'm not sure if I know what the correct behavior should be for labels when the base image for a stage is part of the same build. Either way, the detail should be mentioned in the docs.

I updated the doc to reflect both base stage and base image

nalind

Mostly nits, but I'd like one more test added, and there's an always-true assertion in there by mistake.

imagebuildah/stage_executor.go

tests/bud.bats

nalind · 2025-04-14T15:21:59Z

tests/bud.bats

@@ -2670,6 +2670,69 @@ _EOF
  expect_output "$want_output"
 }

+@test "bud and test inherit-labels" {


Missing calls to _prefetch in here.

tests/bud/base-with-labels/Containerfile.multi-stage uses "alpine" as its base.

I added _prefetch alpine too.

tests/bud.bats

Allows users to specify if they want to inherit labels from base image or not. Signed-off-by: flouthoc <[email protected]>

nalind

LGTM

openshift-ci · 2025-04-14T21:14:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, nalind

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [nalind]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

flouthoc · 2025-04-14T22:24:36Z

@rhatdan @TomSweeneyRedHat PTAL

rhatdan · 2025-04-15T13:19:10Z

Did we ever have a conversation on why not just do --unsetenv=* or --unsetenv=all rather then add a new option?
Perhaps --unsetenv=inherit

--cap-drop=all has precedence.

flouthoc · 2025-04-15T13:44:45Z

@rhatdan I think you mean --unsetlabel=all or --unsetlabel=*. The reason is similar to what I have written here #6098 (comment) , we wanted an option which can only work with labels from base image//base stage but still allow users to add new labels from target stage or CLI. It seems like --unsetlabel=all cannot do this and removes all the labels.

rhatdan · 2025-04-15T17:11:28Z

/lgtm

nalind requested changes Apr 1, 2025

View reviewed changes

TomSweeneyRedHat reviewed Apr 1, 2025

View reviewed changes

flouthoc force-pushed the inherit-labels branch 3 times, most recently from b10e993 to 2df09fd Compare April 7, 2025 19:03

flouthoc requested review from nalind and TomSweeneyRedHat April 9, 2025 03:51

nalind requested changes Apr 9, 2025

View reviewed changes

flouthoc force-pushed the inherit-labels branch 2 times, most recently from 97fe66a to e812133 Compare April 10, 2025 18:30

flouthoc requested a review from nalind April 10, 2025 18:34

nalind requested changes Apr 10, 2025

View reviewed changes

flouthoc force-pushed the inherit-labels branch 2 times, most recently from 1477512 to 04b2894 Compare April 14, 2025 14:22

flouthoc requested a review from nalind April 14, 2025 14:23

nalind requested changes Apr 14, 2025

View reviewed changes

flouthoc force-pushed the inherit-labels branch from 04b2894 to e9b339f Compare April 14, 2025 18:54

flouthoc requested a review from nalind April 14, 2025 18:54

build: add support for inherit-labels

a235033

Allows users to specify if they want to inherit labels from base image or not. Signed-off-by: flouthoc <[email protected]>

flouthoc force-pushed the inherit-labels branch from e9b339f to a235033 Compare April 14, 2025 21:01

nalind approved these changes Apr 14, 2025

View reviewed changes

openshift-ci bot added the approved label Apr 14, 2025

flouthoc requested a review from rhatdan April 15, 2025 15:50

openshift-ci bot assigned rhatdan Apr 15, 2025

openshift-ci bot added the lgtm label Apr 15, 2025

openshift-merge-bot bot merged commit 9a82bcc into containers:main Apr 15, 2025
34 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: add support for `--inherit-labels` #6103

build: add support for `--inherit-labels` #6103

flouthoc commented Apr 1, 2025 •

edited

Loading

Honny1 commented Apr 1, 2025

packit-as-a-service bot commented Apr 1, 2025

nalind left a comment

nalind Apr 1, 2025

nalind Apr 1, 2025

TomSweeneyRedHat Apr 1, 2025

nalind Apr 1, 2025

TomSweeneyRedHat Apr 1, 2025

flouthoc Apr 9, 2025

TomSweeneyRedHat Apr 1, 2025

lsm5 commented Apr 2, 2025

nalind left a comment

nalind Apr 9, 2025

nalind Apr 9, 2025

flouthoc Apr 10, 2025

nalind Apr 10, 2025

flouthoc commented Apr 10, 2025 •

edited

Loading

nalind left a comment

nalind Apr 10, 2025

flouthoc commented Apr 14, 2025

nalind left a comment

nalind Apr 14, 2025

nalind Apr 14, 2025

flouthoc Apr 14, 2025

nalind left a comment

openshift-ci bot commented Apr 14, 2025

flouthoc commented Apr 14, 2025

rhatdan commented Apr 15, 2025 •

edited

Loading

flouthoc commented Apr 15, 2025

rhatdan commented Apr 15, 2025

	// to inheritlabels from base image or not.
	// to inherit labels from base image or not.

	fs.BoolVar(&flags.InheritLabels, "inherit-labels", true, "inherit labels from base image")
	fs.BoolVar(&flags.InheritLabels, "inherit-labels", true, "inherit the labels from the base image")

build: add support for --inherit-labels #6103

build: add support for --inherit-labels #6103

Conversation

flouthoc commented Apr 1, 2025 • edited Loading

What type of PR is this?

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Honny1 commented Apr 1, 2025

packit-as-a-service bot commented Apr 1, 2025

nalind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsm5 commented Apr 2, 2025

nalind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flouthoc commented Apr 10, 2025 • edited Loading

nalind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flouthoc commented Apr 14, 2025

nalind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalind left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Apr 14, 2025

flouthoc commented Apr 14, 2025

rhatdan commented Apr 15, 2025 • edited Loading

flouthoc commented Apr 15, 2025

rhatdan commented Apr 15, 2025

build: add support for `--inherit-labels` #6103

build: add support for `--inherit-labels` #6103

flouthoc commented Apr 1, 2025 •

edited

Loading

flouthoc commented Apr 10, 2025 •

edited

Loading

rhatdan commented Apr 15, 2025 •

edited

Loading