Add support for symbolic chmod options by nickjwhite · Pull Request #6778 · containers/buildah

nickjwhite · 2026-04-09T13:39:48Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This adds an optional Chmod string option to GetOptions and PutOptions. If set, this overrides the ChmodDirs and ChmodFiles options. If unset, they continue to work as before.

The --chmod option to buildah add and buildah copy works with symbolic as well as numeric arguments.

How to verify it

See extra bats tests and copier tests.

Which issue(s) this PR fixes:

Fixes #6066

Special notes for your reviewer:

Note that I'm not sure whether the extra tests in commit_test.go add much value over those in copier/copier_test.go, but I used them while working on this change, so I figured I'd just include them.

Also I'm not sure if this is the way you want the vendored stuff to be included (I added the newly added directory after running 'make vendor') - let me know if I should do it differently.

#6732 suggested calling the new option in GetOptions & PutOptions ChmodStr, but I chose Chmod here because I think it's shorter and clearer.

Does this PR introduce a user-facing change?

The add and copy commands' --chmod argument now accepts symbolic notation, as well as the traditional numeric notation currently supported. Likewise copier's GetOptions and PutOptions now a Chmod option, which accepts numeric or symbolic notation, and if set overrides any ChmodDirs and ChmodFiles options.

packit-as-a-service · 2026-04-09T13:43:38Z

Ephemeral COPR build failed. @containers/packit-build please check.

nalind

Overall this looks very good to me. A couple of quibbles about using test helper functions that would shorten them, and a question about calling mode.Parse() multiple times when reading.

nalind · 2026-04-14T20:52:26Z

 	}
 	// force ownership and/or permissions, if requested
+	if options.Chmod != "" {
+		p, err := mode.Parse(options.Chmod)


We call copierHandlerGetOne() for every item that we're reading. How expensive is this?

Good question. I think the only way to avoid this would be to compute the result of mode.Parse before the calls to copierHandlerGetOne(), and pass it as an argument to the function. As it already takes a lot of arguments I don't love this, as it feels a bit ugly. I haven't benchmarked mode.Parse(), but it looks pretty efficient, exits out early if it just contains a numeric string, and obviously is only called if options.Chmod is set.

However I don't have strong feelings either way, if you think passing the result of mode.Parse() to copierHandlerGetOne() is preferable I'm happy to make the change to make that happen. Or if you have an alternative suggestion.

Yeah, go ahead and initialize the mode.Set in the caller, and pass a pointer to it into copierHandlerGetOne() as an argument (probably with a more descriptive name, "chmod" as the Put handler calls it would be fine) so that it can be compared to nil where it's currently comparing options.Chmod to an empty string.

nickjwhite · 2026-04-15T20:26:35Z

Thanks for the detailed review, @nalind! I'll fix these up tomorrow and push a fresh version.

nickjwhite · 2026-04-15T21:45:53Z

Thanks for the detailed review, @nalind! I'll fix these up tomorrow and push a fresh version.

I ended up not waiting 'til tomorrow and just doing it. Replaced a couple more if statements with assert.Equal while I was there, too.

I'm not sure what to do about the mode.Parse() call in copierHandlerGetOne(), I replied to your comment on that, happy to take your guidance on it.

nalind

Please go ahead and make that change. I think we're going to want this to be rebased before merging.

nalind · 2026-04-20T20:13:38Z

 	}
 	// force ownership and/or permissions, if requested
+	if options.Chmod != "" {
+		p, err := mode.Parse(options.Chmod)


Yeah, go ahead and initialize the mode.Set in the caller, and pass a pointer to it into copierHandlerGetOne() as an argument (probably with a more descriptive name, "chmod" as the Put handler calls it would be fine) so that it can be compared to nil where it's currently comparing options.Chmod to an empty string.

nickjwhite · 2026-04-21T16:23:27Z

Please go ahead and make that change. I think we're going to want this to be rebased before merging.

Done, including the rebase. I haven't tested as heavily, as the tests aren't all working on my local machine as they should, but I'll trust in the CI tests for now.

nalind · 2026-04-21T17:27:18Z

Done, including the rebase. I haven't tested as heavily, as the tests aren't all working on my local machine as they should, but I'll trust in the CI tests for now.

Changes look good, but usually I expect merge commits to be dropped during a rebase. Did you git fetch upstream; git rebase -i upstream/main, or use a different method?

nickjwhite · 2026-04-21T23:48:51Z

Changes look good, but usually I expect merge commits to be dropped during a rebase. Did you git fetch upstream; git rebase -i upstream/main, or use a different method?

Oh, apologies, I said I rebased but I actually just merged main, indeed, my bad. I made a fresh branch from latest main, and cherry-picked & manually reapplied & amended my changes to it. So hopefully this is better.

nalind · 2026-04-22T14:02:12Z

Much, better, thanks!

nalind

LGTM

nalind · 2026-04-22T21:54:52Z

@containers/buildah-maintainers PTAL

TomSweeneyRedHat · 2026-04-23T20:26:07Z

@nickjwhite looks like you may need to rebase this.

TomSweeneyRedHat · 2026-04-23T20:35:18Z

Other than a few test tweaks, and a rebase
LGTM

This adds an optional Chmod string option to GetOptions and PutOptions. If set, this overrides the ChmodDirs and ChmodFiles options. If unset, they continue to work as before. The --chmod option to buildah add and buildah copy works with symbolic as well as numeric arguments. Fixes containers#6066 Signed-off-by: Nick White <git@njw.name>

nickjwhite · 2026-05-01T06:13:41Z

@TomSweeneyRedHat can you take another look please? I added the extra tests you suggested and rebased, so this should be ready to go.

dosubot Bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Apr 9, 2026

nickjwhite force-pushed the better-chmod branch 2 times, most recently from 0d82f4b to 42d5139 Compare April 9, 2026 15:34

nalind reviewed Apr 14, 2026

View reviewed changes

nalind mentioned this pull request Apr 14, 2026

Build definition API checklist #6732

Open

51 tasks

nickjwhite force-pushed the better-chmod branch from 42d5139 to f6861de Compare April 15, 2026 21:11

dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Apr 15, 2026

nickjwhite force-pushed the better-chmod branch from f6861de to 2094dca Compare April 15, 2026 21:19

nalind reviewed Apr 20, 2026

View reviewed changes

nickjwhite force-pushed the better-chmod branch from 57ee0cf to c5cea6e Compare April 21, 2026 13:51

nickjwhite force-pushed the better-chmod branch from 3d13151 to 443ac4d Compare April 21, 2026 23:39

nalind reviewed Apr 22, 2026

View reviewed changes

Comment thread copier/copier.go Outdated

nickjwhite force-pushed the better-chmod branch from 443ac4d to d32a6f7 Compare April 22, 2026 19:38

nalind approved these changes Apr 22, 2026

View reviewed changes

TomSweeneyRedHat reviewed Apr 23, 2026

View reviewed changes

Comment thread tests/copy.bats

nickjwhite force-pushed the better-chmod branch from d32a6f7 to 85942ea Compare April 25, 2026 08:54

nickjwhite force-pushed the better-chmod branch from 85942ea to 7c3cb08 Compare April 26, 2026 20:47

Conversation

nickjwhite commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

packit-as-a-service Bot commented Apr 9, 2026

Uh oh!

nalind left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nalind Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

nickjwhite Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

nalind Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nickjwhite commented Apr 15, 2026

Uh oh!

nickjwhite commented Apr 15, 2026

Uh oh!

nalind left a comment

Choose a reason for hiding this comment

Uh oh!

nalind Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nickjwhite commented Apr 21, 2026

Uh oh!

nalind commented Apr 21, 2026

Uh oh!

nickjwhite commented Apr 21, 2026

Uh oh!

Uh oh!

nalind commented Apr 22, 2026

Uh oh!

nalind left a comment

Choose a reason for hiding this comment

Uh oh!

nalind commented Apr 22, 2026

Uh oh!

TomSweeneyRedHat commented Apr 23, 2026

Uh oh!

Uh oh!

TomSweeneyRedHat commented Apr 23, 2026

Uh oh!

nickjwhite commented May 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nickjwhite commented Apr 9, 2026 •

edited

Loading