Pass cwd for hooks exec run function

[fangpenlin]

What type of PR is this?

For fixing the wrong cwd bug in podman:

containers/podman#18907

/kind bug

What this PR does / why we need it:

I added a new cwd argument for run hooks in common package:

containers/common#1514

for fixing cwd not set bug of poststop hook exec run in podman:

containers/podman#18907

It seems like buildah also depends on the same function and also call hooks exec, so this PR fixes the issue altogether.

How to verify it

Run unit tests

Which issue(s) this PR fixes:

Fixes containers/podman#18907

Special notes for your reviewer:

This PR depends on containers/common#1514 to be merged. Sorry first time contributor, I am not sure what's the correct workflow to make changes in upstream common repo. I assume I should create a new PR in common repo first, then wait for a new release from common then update the vendor file in this repo to make it depends on the changes I made in common?

Does this PR introduce a user-facing change?

Yes, after this PR merged, the cwd of hook exec called by buildah will be the bundle dir (the one containing OCI spec config.json file) instead of current cwd from invoking buildah command.

Hook executables invoked by buildah run command now come with the correct working directory pointing to the container bundle directory

[TomSweeneyRedHat]

/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fangpenlin, TomSweeneyRedHat

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 [TomSweeneyRedHat]

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

[TomSweeneyRedHat]

Now that containers/common#1521 has merged, are there adjustments necessary for this PR?

[fangpenlin]

@TomSweeneyRedHat yeah, changes are needed, I will update it later tonight

[fangpenlin]

@TomSweeneyRedHat I have updated the PR with the new method I added in common lib.
But it seems like a new release was not made yet from there

containers/common#1524

I guess I can only wait until they merge it and make a new one then? By the way, should I check-in vendor changes in my PR? I saw that they were not excluded from the Git repo

[rhatdan]

@fangpenlin This needs a rebase.

[fangpenlin]

@rhatdan Rebased. But I saw there's a //nolint:staticcheck comment seems like disabling lint check or what? Not sure what's it for, should I add the same thing to my changes?

[rhatdan]

I would only worry about the lint check if lint is blowing up. Looks like tests are failing though.

[TomSweeneyRedHat]

@fangpenlin c/common v0.54.0 has been created fwiw. I'm trying to vendor it now into Buildah, but I'm having some machine issues. doing so.

[rhatdan]

@fangpenlin Still working on this?

[fangpenlin]

Oh, sorry, forgot this one. Just rebased

[rhatdan]

LGTM
@flouthoc @giuseppe @vrothberg @nalind @umohnani8 PTAL

[flouthoc]

Commit subject is too long, CI is blocked because of it. Could you please amend commit subject.

[rhatdan]

Remove [NO NEW TESTS NEEDED] from commit message, you should be all set then.

[fangpenlin]

Okay, done. What about now?

[fangpenlin]

@rhatdan the Smoke Test says this

pr-should-include-tests: PR does not include changes in the 'tests' directory

Should I add the [NO NEW TESTS NEEDED] back and make the comment shorter?

[flouthoc]

@fangpenlin bud.bats contains some tests for hooks do you think they can be expected to test this addition ?

[fangpenlin]

@flouthoc

I am not too familiar with buildah code base. But I did a quick search and it seems like the Run method is called from the paths of major buildah functions, like from, run and others:

buildah/cmd/buildah/from.go

Line 169 in 8aa4d36

if err := builder.Run(args, buildah.RunOptions{Stdout: stdout}); err != nil {

buildah/cmd/buildah/run.go

Line 182 in 8aa4d36

runerr := builder.Run(args, options)

buildah/imagebuildah/stage_executor.go

Line 661 in 8aa4d36

err = s.builder.Run(args, options)

So I guess as long as the existing tests run very basic building image stuff, that path should be touched at some point. The only thing missing is that we didn't test specifically for the cwd part. Personally I would rely more on the test cases from upstream project common for this case

https://github.com/containers/common/blob/74c49fcec3ef8a08d0437eea3fea6eb20787a33a/pkg/hooks/exec/exec_test.go#L146-L166

However, if a specific test is really needed, I can find a time to add it. Maybe test against setupOCIHooks? The code lives in run_linux.go, looks like linux specific implementation. Not sure how to write test for it tho, might need to look it up a bit more for that part.

[github-actions]

A friendly reminder that this PR had no activity for 30 days.