Separator is no longer prepended when prefix is empty on podman generate systemd #13299

nirmal-j-patel · 2022-02-20T16:32:01Z

When podman generate systemd is invoked, it previously did not check if
container-prefix or pod-prefix are empty. When these are empty, the file name
starts with the separator, which is hyphen by default. This results in files
like '-containername.service'.

The code now checks if these prefixes are empty. If they are, the filename no
longer adds a separator. Instead, it uses name or ID of the container or pod.

Closes #13272

Signed-off-by: Nirmal Patel npate012@gmail.com

baude · 2022-02-20T17:58:40Z

pkg/systemd/generate/containers.go

+
+	var serviceName string
+	if options.ContainerPrefix == "" {
+		serviceName = nameOrID


would you consider serviceName := NameOrID and then use a single IF condition for options.ContainerPrefix != "". Also, personal thing, but i prefer len(options.ContainerPrefix) == 0 over "".

I think it would be better to put this into a small function and use this function for container and pod name,
e.g.

function getServiceName(prefix, sperator, name) string { if prefix == "" { return name } return fmt.Sprintf("%s%s%s", prefix, separator, name) } serviceName := getServiceName(...)

This way would do not need to duplicate the logic

I have created a function for this. I also used + to concatenate string instead of Sprintf because fmt was not imported in common.go and I wasn't sure if I should import it only for this function. I also changed the code to use a single if.

baude · 2022-02-20T17:59:03Z

pkg/systemd/generate/pods.go

+	var serviceName string
+	if options.PodPrefix == "" {
+		serviceName = nameOrID
+	} else {


same comment about reducing this down to one conditional

Updated the code to use a function which has a single if condition

baude · 2022-02-20T18:00:25Z

test/e2e/generate_systemd_test.go

@@ -423,6 +423,20 @@ var _ = Describe("Podman generate systemd", func() {

 	})

+	It("podman generate systemd --container-prefix ''", func() {


@vrothberg PTAL at the tests added.

I ran the test using GOPATH=~/go go test -v test/e2e/libpod_suite_test.go test/e2e/common_test.go test/e2e/config.go test/e2e/config_amd64.go test/e2e/generate_systemd_test.go

and I did not see any failure. So I pushed those changed. Hopefully there will not be any failures in CI. The following is the summary of the output of command.

Ran 32 of 32 Specs in 429.065 seconds SUCCESS! -- 32 Passed | 0 Failed | 0 Pending | 0 Skipped --- PASS: TestLibpod (429.07s) PASS ok command-line-arguments 429.114s

mheon · 2022-02-20T18:23:04Z

/approve

Luap99 · 2022-02-20T21:12:23Z

Also please add a unit test in containers_test.go and pods_test.go.

vrothberg · 2022-02-21T09:39:06Z

test/e2e/generate_systemd_test.go

+		n.WaitWithDefaultTimeout()
+		Expect(n).Should(Exit(0))
+
+		session := podmanTest.Podman([]string{"generate", "systemd", "--container-prefix", "", "--pod-prefix", "", "--separator", "_", "--name", "foo"})


Could you merge the two pod tests? This way, the pod and its containers are created once and we save some test cycles.

I merged the two pod tests to a single test. Thank you for the suggestion.

nirmal-j-patel · 2022-02-21T22:45:36Z

Thank you everyone for your suggestions. I will fix these issues and push the changes.

nirmal-j-patel · 2022-02-23T18:04:09Z

Also please add a unit test in containers_test.go and pods_test.go.

I have added tests to containers_test.go and pods_test.go. Hopefully they look good.

TomSweeneyRedHat · 2022-02-23T22:21:59Z

@npate012 thanks for the PR. It looks like your change may have upset some other tests.

Luap99 · 2022-02-24T10:40:55Z

The podman-remote tests are failing, you need to make sure that podman-remote generate systemd ... will also work.
The API endpoint handler is here:

podman/pkg/api/handlers/libpod/generate.go

Line 16 in 0d2bd53

func GenerateSystemd(w http.ResponseWriter, r *http.Request) {

The client binding is here:

podman/pkg/bindings/generate/generate.go

Line 12 in 0d2bd53

    
           func Systemd(ctx context.Context, nameOrID string, options *SystemdOptions) (*entities.GenerateSystemdReport, error) {

Either the client or server is not using the empty prefix.

Luap99 · 2022-03-10T14:46:53Z

pkg/api/handlers/libpod/generate.go

-		ContainerPrefix: "container",
-		PodPrefix:       "pod",
-		Separator:       "-",


I understand why you changed this but this will break api users, who rely on this default.

I think the correct fix would be to make the fields a pointer (*string).

I appreciate your review and help. Could you please expand on how making the fields a pointer would work? I am not able to understand this strategy. Thank you.

Sorry this comment got lost in the inbox.
change this to

ContainerPrefix *string `schema:"containerPrefix"` PodPrefix *string `schema:"podPrefix"` Separator *string `schema:"separator"`

Leave the default unset.

Now in the code below you can check if var == nil, in this case you set the correct default of container, pod, -.
If not nil you can just use the value.

Note this is just my theory I have not tested if this will actually work. But this change will allow us to differentiate between an empty string ("") or not set at all (nil).

I made this change and it works.

…ate systemd When podman generate systemd is invoked, it previously did not check if container-prefix or pod-prefix are empty. When these are empty, the file name starts with the separator, which is hyphen by default. This results in files like '-containername.service'. The code now checks if these prefixes are empty. If they are, the filename no longer adds a separator. Instead, it uses name or ID of the container or pod. Closes containers#13272 Signed-off-by: Nirmal Patel <npate012@gmail.com>

nirmal-j-patel · 2022-03-17T02:23:44Z

Thanks everyone for your help. I feel this pull request is ready for review. All tests pass successfully. I have changed the status from draft to ready for review.

vrothberg

LGTM
@Luap99 PTanotherL

vrothberg · 2022-03-17T08:08:40Z

Thanks for contributing, @npate012

Luap99

LGTM, one minor nit but it is not worth to repush for this

Luap99 · 2022-03-17T09:30:55Z

pkg/api/handlers/libpod/generate.go

 	}

 	if err := decoder.Decode(&query, r.URL.Query()); err != nil {
 		utils.Error(w, http.StatusBadRequest, errors.Wrapf(err, "failed to parse parameters for %s", r.URL.String()))
 		return
 	}

+	var ContainerPrefix = "container"


the idiomatic go way would be to write containerPrefix := "container", also local variables should start lower case.

openshift-ci · 2022-03-17T09:31:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, mheon, npate012

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~~ [Luap99,mheon]

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

Luap99 · 2022-03-17T09:31:33Z

/lgtm

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 20, 2022

baude reviewed Feb 20, 2022

View reviewed changes

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

vrothberg reviewed Feb 21, 2022

View reviewed changes

nirmal-j-patel force-pushed the fix_systemd_generate_name_on_empty_prefix branch from 4857269 to e84e667 Compare February 23, 2022 17:57

nirmal-j-patel force-pushed the fix_systemd_generate_name_on_empty_prefix branch from e84e667 to a51f7f8 Compare March 5, 2022 22:23

Luap99 reviewed Mar 10, 2022

View reviewed changes

nirmal-j-patel force-pushed the fix_systemd_generate_name_on_empty_prefix branch from a51f7f8 to 4b144a3 Compare March 17, 2022 00:04

nirmal-j-patel force-pushed the fix_systemd_generate_name_on_empty_prefix branch from 4b144a3 to 714e5a1 Compare March 17, 2022 00:27

nirmal-j-patel marked this pull request as ready for review March 17, 2022 02:22

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 17, 2022

vrothberg reviewed Mar 17, 2022

View reviewed changes

Luap99 approved these changes Mar 17, 2022

View reviewed changes

openshift-ci bot assigned Luap99 Mar 17, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 17, 2022

openshift-merge-robot merged commit 6ce9677 into containers:main Mar 17, 2022

nirmal-j-patel deleted the fix_systemd_generate_name_on_empty_prefix branch July 3, 2022 20:23

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 Sep 21, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separator is no longer prepended when prefix is empty on podman generate systemd #13299

Separator is no longer prepended when prefix is empty on podman generate systemd #13299

nirmal-j-patel commented Feb 20, 2022

baude Feb 20, 2022

Luap99 Feb 20, 2022

nirmal-j-patel Feb 23, 2022

baude Feb 20, 2022

nirmal-j-patel Feb 23, 2022

baude Feb 20, 2022

nirmal-j-patel Feb 23, 2022

mheon commented Feb 20, 2022

Luap99 commented Feb 20, 2022

vrothberg Feb 21, 2022

nirmal-j-patel Feb 23, 2022

nirmal-j-patel commented Feb 21, 2022

nirmal-j-patel commented Feb 23, 2022

TomSweeneyRedHat commented Feb 23, 2022

Luap99 commented Feb 24, 2022

Luap99 Mar 10, 2022

nirmal-j-patel Mar 12, 2022

Luap99 Mar 16, 2022

nirmal-j-patel Mar 17, 2022

nirmal-j-patel commented Mar 17, 2022

vrothberg left a comment

vrothberg commented Mar 17, 2022

Luap99 left a comment

Luap99 Mar 17, 2022

openshift-ci bot commented Mar 17, 2022

Luap99 commented Mar 17, 2022

		@@ -423,6 +423,20 @@ var _ = Describe("Podman generate systemd", func() {

		})

		It("podman generate systemd --container-prefix ''", func() {

Separator is no longer prepended when prefix is empty on podman generate systemd #13299

Separator is no longer prepended when prefix is empty on podman generate systemd #13299

Conversation

nirmal-j-patel commented Feb 20, 2022

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

mheon commented Feb 20, 2022

Luap99 commented Feb 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirmal-j-patel commented Feb 21, 2022

nirmal-j-patel commented Feb 23, 2022

TomSweeneyRedHat commented Feb 23, 2022

Luap99 commented Feb 24, 2022

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

nirmal-j-patel commented Mar 17, 2022

vrothberg left a comment

Choose a reason for hiding this comment

vrothberg commented Mar 17, 2022

Luap99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Mar 17, 2022

Luap99 commented Mar 17, 2022