Add support for symbolic chmod options

[nickjwhite]

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]

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.

[nickjwhite]

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

[nickjwhite]

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.

[nickjwhite]

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]

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]

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]

Much, better, thanks!

[nalind]

LGTM

[nalind]

@containers/buildah-maintainers PTAL

[TomSweeneyRedHat]

@nickjwhite looks like you may need to rebase this.

[TomSweeneyRedHat]

Other than a few test tweaks, and a rebase
LGTM

[nickjwhite]

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

[TomSweeneyRedHat]

@nickjwhite apologies for the very tardy re-review. LGTM.
However, it looks like you need to rebase before we can commit.