podman: Added find slirp4netns binary file from helper_binaries_dir #18620

HirazawaUi · 2023-05-18T16:03:04Z

[NO NEW TESTS NEEDED]

Does this PR introduce a user-facing change?

If --network-cmd-path is not set, then slirp4netns binary will first be searched using the `helper_binaries_dir` option in `containers.conf`, and second using the `$ PATH` environment variable.

rhatdan · 2023-05-18T16:59:09Z

You must sign your commits.
git commit -a --amend -s
git push --force

Also does this require any man page changes?

@Luap99 PTAL

PhrozenByte · 2023-05-18T18:54:56Z

Thanks @HirazawaUi 👍

Can you please also add deprecation notices to uses of r.config.Engine.NetworkCmdPath? Like the following (that should be updated, too):

podman/cmd/podman/root.go

Line 441 in a58ea23

    
           // TODO (5.0): remove this option with the next major release after https://github.com/containers/podman/issues/18560 was implemented

Also does this require any man page changes?

Yes, that would be appreciated as per discussion in #18560. To be more precise: The docs should explain the fallback to helper_binaries_dir, and that --network-cmd-path is a deprecated option that will be removed with the next major release (whether Podman 5.0 really drops it is something to decide when Podman 5.0 is actually on its way)

podman/docs/source/markdown/podman.1.md

Lines 85 to 86 in a58ea23

    
           #### **--network-cmd-path**=*path* 
        
           Path to the `slirp4netns(1)` command binary to use for setting up a slirp4netns network.  If "" is used then the binary is looked up using the $PATH environment variable.

Luap99

code change LGTM but I would like to see the documentation change as mentioned by @PhrozenByte

TomSweeneyRedHat · 2023-05-19T15:51:51Z

Changes LGTM
But one of your commits appears to be unsigned and needs to be signed. Added doc is always good.

HirazawaUi · 2023-05-19T18:02:43Z

Thanks for the reminder, I have finished the signature and modified the docs

HirazawaUi · 2023-05-19T18:14:16Z

Can you please also add deprecation notices to uses of r.config.Engine.NetworkCmdPath? Like the following (that should be updated, too):

There are so many places to use r.config.Engine.NetworkCmdPath that it seems too tedious to add deprecation notices to each one, so I chose to add a section after the TODO in cmd/podman/root.go that tells us we need to clean up each one one after we finish Code that uses r.config.Engine.NetworkCmdPath, do you like it this way?

PhrozenByte

Thanks @HirazawaUi

Just some docs stuff. LGTM besides those small things 👍

Note: I can't say anything about the actual code changes 😉

PhrozenByte · 2023-05-19T17:50:13Z

docs/source/markdown/podman.1.md

@@ -83,7 +83,8 @@ Remote connections use local containers.conf for default.
 Log messages at and above specified level: debug, info, warn, error, fatal or panic (default: "warn")

 #### **--network-cmd-path**=*path*
-Path to the `slirp4netns(1)` command binary to use for setting up a slirp4netns network.  If "" is used then the binary is looked up using the $PATH environment variable.
+Path to the `slirp4netns(1)` command binary to use for setting up a slirp4netns network.  If "" is used, then the binary will be searched for from the helper_binaries_dir.


Suggested change

Path to the `slirp4netns(1)` command binary to use for setting up a slirp4netns network. If "" is used, then the binary will be searched for from the helper_binaries_dir.

Path to the `slirp4netns(1)` command binary to use for setting up a slirp4netns network. If "" is used, then the binary will first be searched using the `helper_binaries_dir` option in `containers.conf`, and second using the `$PATH` environment variable.

Explain what helper_binaries_dir is and still mention $PATH.

Good suggestion

PhrozenByte · 2023-05-19T17:51:17Z

docs/source/markdown/podman.1.md

@@ -83,7 +83,8 @@ Remote connections use local containers.conf for default.
 Log messages at and above specified level: debug, info, warn, error, fatal or panic (default: "warn")

 #### **--network-cmd-path**=*path*
-Path to the `slirp4netns(1)` command binary to use for setting up a slirp4netns network.  If "" is used then the binary is looked up using the $PATH environment variable.
+Path to the `slirp4netns(1)` command binary to use for setting up a slirp4netns network.  If "" is used, then the binary will be searched for from the helper_binaries_dir.
+It will be deprecated in Podman 5.0.


Suggested change

It will be deprecated in Podman 5.0.

**Note:** This option is deprecated and will be removed with Podman 5.0. Use the `helper_binaries_dir` option in `containers.conf` instead.

The option is deprecated as of now (resp. the next version this happens to get merged into); it might be removed with Podman 5.0.

Just thinking about it: Doesn't this imply a user facing change?

Cc: @Luap99

Good suggestion, this is supposed to be a user facing change, I will add it later

PhrozenByte · 2023-05-19T17:52:10Z

cmd/podman/root.go

@@ -439,6 +439,7 @@ func rootFlags(cmd *cobra.Command, podmanConfig *entities.PodmanConfig) {
 		_ = cmd.RegisterFlagCompletionFunc(conmonFlagName, completion.AutocompleteDefault)

 		// TODO (5.0): remove this option with the next major release after https://github.com/containers/podman/issues/18560 was implemented


Suggested change

// TODO (5.0): remove this option with the next major release after https://github.com/containers/podman/issues/18560 was implemented

// TODO (5.0): --network-cmd-path is deprecated, consider removing it

There's no need to still reference the issue. You can also merge this with the next line if you want to.

I agree that adding notices to every use is a bit tedious...

I agree not to reference this issue, but I think --network-cmd-path is deprecated, remove this option with the next major release would be better.

[NO NEW TESTS NEEDED] Fixes: containers#18568 Signed-off-by: binghongtao <[email protected]>

PhrozenByte · 2023-05-19T19:24:56Z

LGTM, thanks @HirazawaUi for your work and considering the suggestions! 👍

rhatdan · 2023-05-19T21:35:41Z

/aprove
/lgtm
Thanks @HirazawaUi

rhatdan · 2023-05-20T10:14:01Z

/approve

openshift-ci · 2023-05-20T10:14:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HirazawaUi, rhatdan

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~~ [rhatdan]

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

openshift-ci bot added the release-note-none label May 18, 2023

Luap99 reviewed May 19, 2023

View reviewed changes

HirazawaUi force-pushed the find_slirp4netns_from_helper_binaries_dir branch 3 times, most recently from b7fb86e to 258e87c Compare May 19, 2023 18:00

PhrozenByte reviewed May 19, 2023

View reviewed changes

podman: Added find slirp4netns binary file from helper_binaries_dir

2974936

[NO NEW TESTS NEEDED] Fixes: containers#18568 Signed-off-by: binghongtao <[email protected]>

HirazawaUi force-pushed the find_slirp4netns_from_helper_binaries_dir branch from 258e87c to 2974936 Compare May 19, 2023 19:17

openshift-ci bot added release-note and removed release-note-none labels May 19, 2023

openshift-ci bot assigned rhatdan May 19, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 19, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 20, 2023

openshift-merge-robot merged commit a829122 into containers:main May 20, 2023

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Aug 23, 2023

github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

podman: Added find slirp4netns binary file from helper_binaries_dir #18620

podman: Added find slirp4netns binary file from helper_binaries_dir #18620

HirazawaUi commented May 18, 2023 •

edited

Loading

rhatdan commented May 18, 2023

PhrozenByte commented May 18, 2023 •

edited

Loading

Luap99 left a comment

TomSweeneyRedHat commented May 19, 2023

HirazawaUi commented May 19, 2023

HirazawaUi commented May 19, 2023

PhrozenByte left a comment

PhrozenByte May 19, 2023

HirazawaUi May 19, 2023

PhrozenByte May 19, 2023

PhrozenByte May 19, 2023

HirazawaUi May 19, 2023

PhrozenByte May 19, 2023 •

edited

Loading

HirazawaUi May 19, 2023

PhrozenByte commented May 19, 2023

rhatdan commented May 19, 2023

rhatdan commented May 20, 2023

openshift-ci bot commented May 20, 2023

	Path to the `slirp4netns(1)` command binary to use for setting up a slirp4netns network. If "" is used, then the binary will be searched for from the helper_binaries_dir.
	Path to the `slirp4netns(1)` command binary to use for setting up a slirp4netns network. If "" is used, then the binary will first be searched using the `helper_binaries_dir` option in `containers.conf`, and second using the `$PATH` environment variable.

	It will be deprecated in Podman 5.0.
	Note: This option is deprecated and will be removed with Podman 5.0. Use the `helper_binaries_dir` option in `containers.conf` instead.

		@@ -439,6 +439,7 @@ func rootFlags(cmd cobra.Command, podmanConfig entities.PodmanConfig) {
		_ = cmd.RegisterFlagCompletionFunc(conmonFlagName, completion.AutocompleteDefault)

		// TODO (5.0): remove this option with the next major release after https://github.com/containers/podman/issues/18560 was implemented

	// TODO (5.0): remove this option with the next major release after https://github.com/containers/podman/issues/18560 was implemented
	// TODO (5.0): --network-cmd-path is deprecated, consider removing it

podman: Added find slirp4netns binary file from helper_binaries_dir #18620

podman: Added find slirp4netns binary file from helper_binaries_dir #18620

Conversation

HirazawaUi commented May 18, 2023 • edited Loading

Does this PR introduce a user-facing change?

rhatdan commented May 18, 2023

PhrozenByte commented May 18, 2023 • edited Loading

Luap99 left a comment

Choose a reason for hiding this comment

TomSweeneyRedHat commented May 19, 2023

HirazawaUi commented May 19, 2023

HirazawaUi commented May 19, 2023

PhrozenByte left a comment

Choose a reason for hiding this comment

PhrozenByte May 19, 2023

Choose a reason for hiding this comment

HirazawaUi May 19, 2023

Choose a reason for hiding this comment

PhrozenByte May 19, 2023

Choose a reason for hiding this comment

PhrozenByte May 19, 2023

Choose a reason for hiding this comment

HirazawaUi May 19, 2023

Choose a reason for hiding this comment

PhrozenByte May 19, 2023 • edited Loading

Choose a reason for hiding this comment

HirazawaUi May 19, 2023

Choose a reason for hiding this comment

PhrozenByte commented May 19, 2023

rhatdan commented May 19, 2023

rhatdan commented May 20, 2023

openshift-ci bot commented May 20, 2023

HirazawaUi commented May 18, 2023 •

edited

Loading

PhrozenByte commented May 18, 2023 •

edited

Loading

PhrozenByte May 19, 2023 •

edited

Loading