Pretty print systemd services file #13814
Conversation
openshift-ci
bot
added
the
approved
|
@Luap99 @vrothberg PTAL |
|
All kinds of test unhappiness @rhatdan |
pkg/systemd/generate/containers.go
Outdated
| @@ -282,10 +282,23 @@ func setContainerNameForTemplate(startCommand []string, info *containerInfo) ([] | |||
| return startCommand, nil | |||
| } | |||
|
|
|||
| func formatOptions(options []string) string { | |||
| var formatted string | |||
There was a problem hiding this comment.
please use a string builder for this
pkg/systemd/generate/containers.go
Outdated
| // executeContainerTemplate executes the container template on the specified | ||
| // containerInfo. Note that the containerInfo is also post processed and | ||
| // completed, which allows for an easier unit testing. | ||
| func executeContainerTemplate(info *containerInfo, options entities.GenerateSystemdOptions) (string, error) { | ||
| fmt.Println("DAN Start") |
rhatdan
force-pushed
the
systemd
branch
3 times, most recently
from
e5052af to
1ef80bb
Compare
pkg/systemd/generate/containers.go
Outdated
| if formatted.Len() == 0 { | ||
| formatted.WriteString(o) | ||
| } else { | ||
| formatted.WriteString(" " + o) | ||
| } |
There was a problem hiding this comment.
we can remove this branch here if we loop over options[1:] and add the first argument before the loop. I think it is safe to assume that we are always called with at least one argument.
pkg/systemd/generate/containers.go
Outdated
| return "" | ||
| } | ||
| formatted.WriteString(options[0]) | ||
| for _, o := range options[:1] { |
There was a problem hiding this comment.
| for _, o := range options[:1] { | |
| for _, o := range options[1:] { |
| -d \ | ||
| --replace \ | ||
| --name test awesome-image:latest sh \ | ||
| -c "kill $$$$ && echo %%\\" |
There was a problem hiding this comment.
| -c "kill $$$$ && echo %%\\" | |
| sh -c "kill $$$$ && echo %%\\" |
and remove "sh" from line 637
There was a problem hiding this comment.
That is difficult/impossible to do, would need to figure out the first line of the command. Recall these are commands within commands. The command is not processed by Podman so there is no way to differentiate between the command/image and subcommand.
There was a problem hiding this comment.
It is possible to do this but it would require way more logic. I think the current check is good enough.
There was a problem hiding this comment.
While possible for certain use cases, like podman in podman, I am not sure we would be 100 successful in interpreting these type of commands imagine running chroot or sudo within a container, or even bash -c. It would get way more complex for not much benefit.
| --cgroups=no-conmon \ | ||
| --rm \ | ||
| --sdnotify=conmon \ | ||
| -d awesome-image:latest |
There was a problem hiding this comment.
Just curious why you didn't have the argument(s) on their own line for each of these ala:
| -d awesome-image:latest | |
| -d \ | |
| awesome-image:latest |
There was a problem hiding this comment.
The original patch did this, and it was felt that this did not look good, it broke the line up into two many lines. @Luap99 and I agreed this method looked best.
Fixes: containers#13337 I added newline only on options IE Begin with "-" [NO NEW TESTS NEEDED] Signed-off-by: Abhijeet Kasurde <[email protected]> Signed-off-by: Daniel J Walsh <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flouthoc, 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:
Approvers can indicate their approval by writing |
github-actions
bot
added
the
locked - please file new issue/PR
Fixes: #13337
I added newline only on options IE Begin with "-"
[NO NEW TESTS NEEDED]
Signed-off-by: Abhijeet Kasurde [email protected]
Signed-off-by: Daniel J Walsh [email protected]
Replaces: #13680