No header info for systemd generation #9445

jmguzik · 2021-02-19T23:10:04Z

No header info for systemd generation: --no-header parameter
Fixes #8880
Closes #8880

vrothberg

Thanks for tackling it!

Just a couple of nits. Could you also add two tests to test/e2e/generate_systemd_test.go that uses both --no-header=true and --no-header=false?

cmd/podman/generate/systemd.go

docs/source/markdown/podman-generate-systemd.1.md

vrothberg · 2021-02-20T09:05:00Z

pkg/systemd/generate/common.go

@@ -30,18 +30,23 @@ func validateRestartPolicy(restart string) error {
 	return errors.Errorf("%s is not a valid restart policy", restart)
 }

-const headerTemplate = `# {{{{.ServiceName}}}}.service
-# autogenerated by Podman {{{{.PodmanVersion}}}}
+const serviceTemplate = `# {{{{.ServiceName}}}}.service


I prefer to use a conditional, which is more template idiomatic:

const headerTemplate = `# {{{{.ServiceName}}}}.service {{{- if.GenerateHeader}}} # autogenerated by Podman {{{{.PodmanVersion}}}} {{{{- if .TimeStamp}}}} # {{{{.TimeStamp}}}} {{{{- end}}}} {{{{- end}}}} [Unit] Description=Podman {{{{.ServiceName}}}}.service Documentation=man:podman-generate-systemd(1) Wants=network.target After=network-online.target `

Hmm nice one, will see how it works

Works nicely, I admit didn't know about this (fairly new to go). However, I changed logic to GenerateNoHeader. Firstly, because the flag is called --no-header, and secondly because the default value will be false. Then, there is no need to add it to every structure and set explicitly true, as well as to tests. Hope it's ok for you.

jmguzik · 2021-02-20T09:29:24Z

Just a couple of nits. Could you also add two tests to test/e2e/generate_systemd_test.go that uses both --no-header=true and --no-header=false?

@vrothberg --no-header currently similar to --new flag. It does not take =value. You just want to check what will happen or implement is as =value?

If first, what results do you expect? Error?

vrothberg · 2021-02-20T10:23:54Z

@vrothberg --no-header currently similar to --new flag. It does not take =value. You just want to check what will happen or implement is as =value?

Both take arguments such as ./bin/podman generate systemd --no-header=false --new=false $CTR. Setting it false in the tests would just make sure that this value behaves as expected. I don't think we test that yet for --new=false though.

rhatdan · 2021-02-21T10:08:21Z

pkg/systemd/generate/common.go

@@ -31,10 +31,12 @@ func validateRestartPolicy(restart string) error {
 }

 const headerTemplate = `# {{{{.ServiceName}}}}.service


What do you think about making this part of containers.conf and allowing users to edit it? Then we would not need to add all of the plumbing for a new option and users could just modify the default template however they wanted?

@vrothberg Thoughts?

I would say it would be another feature - making this code useless.
It is architecture decision and you have to discuss this before I move further.
So @rhatdan , @vrothberg please let me know.

I think supporting this kind of use case would be even more difficult. This would no longer allow us to do breaking changes to the template such as #9035.
I think adding a simple cli option is simple and intuitive for users.

What do you think about making this part of containers.conf and allowing users to edit it? Then we would not need to add all of the plumbing for a new option and users could just modify the default template however they wanted?

I think we always need a CLI counterpart for containers.conf options such that users can override defaults. I have not strong feeling whether it's a good candidate for containers.conf.

But, I think it's something orthogonal to this change and we can add that a later point.

@jmguzik feel free to continue working on it. It would not change the architecture or logic of this PR. If we want a counterpart in the containers.conf, we can add it separately.

@vrothberg Ok, thanks for the clarification!

jmguzik · 2021-02-22T09:25:03Z

Generally, this feature is ready but remote test cases are failing. I suspect that something (probably one, two lines) is missing to enable the remote. I can't find it though - not super familiar with the codebase. Could I ask for a little help to spot the place @Luap99 @vrothberg ?

Luap99 · 2021-02-22T09:33:12Z

For remote you have to add the option here:

podman/pkg/bindings/generate/types.go

Line 12 in 4a6582b

type SystemdOptions struct {

and run make generate-bindings afterwards. Lastly, you have to add: WithNoHeaders here

podman/pkg/domain/infra/tunnel/generate.go

Line 12 in 4a6582b

    
           options.WithPodPrefix(opts.PodPrefix).WithRestartPolicy(opts.RestartPolicy).WithSeparator(opts.Separator)

vrothberg · 2021-02-22T09:48:30Z

In addition to what @Luap99 mentioned, there's a bit more to do. The Swagger documentation, the request routing and the handling of remote requests is all handled in pkg/api/*. So to make remote work, we also need to;

Add the new noHeader remote parameter to the docs in pkg/api/server/register_generate.go.
Supported the new parameter in the handler in pkg/api/handlers/libpod/generate.go.

It's mostly plumbing work to wire in the new parameter. The Podman backend is largely structured into two implementations of the same interfaces. One for the native local Linux one (pkg/domain/infra/abi) and another one for the remote clients (pkg/domain/infra/tunnel). The remote code is itself using the go-bindings we maintain directly in this repository in pkg/bindings.

@Luap99's comments cover the remote-client side. Mine above cover the server part.

jmguzik · 2021-02-22T12:31:40Z

@vrothberg @Luap99 without help, it would take me some time to find this. Many thanks for the insights. From my perspective feature is ready to be reviewed.

vrothberg · 2021-02-22T13:42:23Z

@vrothberg @Luap99 without help, it would take me some time to find this. Many thanks for the insights. From my perspective feature is ready to be reviewed.

LGTM. Can you squash the four commits into one?

vrothberg · 2021-02-22T13:43:18Z

Thanks for tackling it! If you're interested in contributing more, feel free to have a look at the open issue or reach out to us directly. There is always work :)

Signed-off-by: Jakub Guzik <[email protected]>

jmguzik · 2021-02-22T13:54:29Z

LGTM. Can you squash the four commits into one?

Squashed :)

Thanks for tackling it! If you're interested in contributing more, feel free to have a look at the open issue or reach out to us directly. There is always work :)

Sure :) My idea is to learn Go (my background is many years mainly C/C++). This is a good way. I will take a look in my free time, but I have still one PR open in buildah. I will try to finish this one first.

Luap99

LGTM

openshift-ci-robot · 2021-02-22T14:55:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jmguzik, Luap99

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]

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

Luap99 · 2021-02-22T14:55:18Z

/lgtm
/hold

Luap99 · 2021-02-22T18:43:36Z

/hold cancel

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

jmguzik force-pushed the no-header-info-for-systemd-generation branch from f68f46f to f7a60a2 Compare February 19, 2021 23:35

vrothberg reviewed Feb 20, 2021

View reviewed changes

jmguzik force-pushed the no-header-info-for-systemd-generation branch 3 times, most recently from 65eac05 to 9d5d20d Compare February 21, 2021 00:24

rhatdan reviewed Feb 21, 2021

View reviewed changes

jmguzik force-pushed the no-header-info-for-systemd-generation branch 3 times, most recently from e0bb17d to 4595512 Compare February 21, 2021 13:39

jmguzik mentioned this pull request Feb 21, 2021

Two usage variations of --new flag added to e2e #9458

Merged

jmguzik changed the title ~~[WIP] No header info for systemd generation~~ No header info for systemd generation Feb 22, 2021

jmguzik marked this pull request as ready for review February 22, 2021 12:32

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 22, 2021

--no-header flag implementation for generate systemd

d2f3098

Signed-off-by: Jakub Guzik <[email protected]>

jmguzik force-pushed the no-header-info-for-systemd-generation branch from f1e5f0f to d2f3098 Compare February 22, 2021 13:49

Luap99 approved these changes Feb 22, 2021

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 22, 2021

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 22, 2021

openshift-ci-robot assigned Luap99 Feb 22, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 22, 2021

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 22, 2021

openshift-merge-robot merged commit a6e7d19 into containers:master Feb 22, 2021

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 23, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

No header info for systemd generation #9445

No header info for systemd generation #9445

jmguzik commented Feb 19, 2021 •

edited

Loading

vrothberg left a comment

vrothberg Feb 20, 2021

jmguzik Feb 20, 2021

jmguzik Feb 22, 2021

jmguzik commented Feb 20, 2021

vrothberg commented Feb 20, 2021

rhatdan Feb 21, 2021

jmguzik Feb 21, 2021

Luap99 Feb 22, 2021

vrothberg Feb 22, 2021

vrothberg Feb 22, 2021

jmguzik Feb 22, 2021

jmguzik commented Feb 22, 2021

Luap99 commented Feb 22, 2021

vrothberg commented Feb 22, 2021

jmguzik commented Feb 22, 2021 •

edited

Loading

vrothberg commented Feb 22, 2021

vrothberg commented Feb 22, 2021

jmguzik commented Feb 22, 2021

Luap99 left a comment

openshift-ci-robot commented Feb 22, 2021

Luap99 commented Feb 22, 2021

Luap99 commented Feb 22, 2021

		@@ -31,10 +31,12 @@ func validateRestartPolicy(restart string) error {
		}

		const headerTemplate = `# {{{{.ServiceName}}}}.service

No header info for systemd generation #9445

No header info for systemd generation #9445

Conversation

jmguzik commented Feb 19, 2021 • edited Loading

vrothberg left a 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

jmguzik commented Feb 20, 2021

vrothberg commented Feb 20, 2021

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

jmguzik commented Feb 22, 2021

Luap99 commented Feb 22, 2021

vrothberg commented Feb 22, 2021

jmguzik commented Feb 22, 2021 • edited Loading

vrothberg commented Feb 22, 2021

vrothberg commented Feb 22, 2021

jmguzik commented Feb 22, 2021

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Feb 22, 2021

Luap99 commented Feb 22, 2021

Luap99 commented Feb 22, 2021

jmguzik commented Feb 19, 2021 •

edited

Loading

jmguzik commented Feb 22, 2021 •

edited

Loading