Fix go template parsing with "\n" in it #15673

Luap99 · 2022-09-07T13:54:21Z

update all commands to use the report.Formatter
update c/common with the latest version so that report.Formatter can handle newlines
see individual commit for more information

Fixes #13446
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=2059658

Does this PR introduce a user-facing change?

The format flag now handles template strings with `\n` in it correctly.

openshift-ci · 2022-09-07T13:54:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 · 2022-09-07T13:56:50Z

/hold for tests, ideally we can add a test which makes sure all --format flags work correctly since I had to change some commands manually, something like --format '{{printf "\n"}}' should work with all commands.

cc @edsantiago

mheon · 2022-09-07T18:21:18Z

Linter is angry

edsantiago · 2022-09-07T18:22:09Z

still a WIP. I'm working on tests for it.

edsantiago · 2022-09-07T18:47:20Z

Luap99#3

edsantiago · 2022-09-08T12:19:21Z

See treadmill PR #13808 for fixes to the vendor bugs

jwhonce · 2022-09-08T16:46:55Z

cmd/podman/machine/list.go

-		row = cmd.Flag("format").Value.String()
-		printHeader = report.HasTable(row)
-		row = report.NormalizeFormat(row)
+	case cmd.Flag("format").Changed:


FYI cmd.Flags().Changed("format") guards against using a nil pointer while cmd.Flag("format").Changed does not. Only exposed when flag is undefined.

I don't think that matters much since a) we know that the flag exists and b) this PR includes a test for all format flags so if it would panic the test will catch it.

edsantiago · 2022-09-09T15:43:10Z

Sorry about the test failure. Since my first iteration, I reworked the test to make it better; was going to submit it as a followup PR, but could you grab it here instead?
610-format.bats.txt

This version does a much better job of error reporting; it also is more thorough, as in, it catches podman container inspect which still seems to need fixing.

Luap99 · 2022-09-12T09:46:25Z

I applied your test changes but I see no errors with podman container inspect (Note that container inspect uses the same code as most other inspect commands, so if one fails more of them should fail as well)

edsantiago · 2022-09-12T12:03:45Z

Sorry about the churn. This fixes the rootless hang (because of empty events file) and the stupid, nonworking root-machine check:

diff --git a/test/system/610-format.bats b/test/system/610-format.bats
index c950827b4..287a29393 100644
--- a/test/system/610-format.bats
+++ b/test/system/610-format.bats
@@ -52,7 +52,7 @@ function check_subcommand() {                                                                                                                
     for cmd in $(_podman_commands "$@"); do
         # Special case: 'podman machine' can't be run as root. No override.
         if [[ "$cmd" = "machine" ]]; then
-            if is_root; then
+            if ! is_rootless; then
                 continue
             fi
         fi
@@ -136,7 +136,7 @@ function check_subcommand() {                                                                                                              
     done < <(parse_table "$extra_args_table")
 
     # Setup: some commands need a container, pod, machine, or secret
-    run_podman run -d --name mycontainer $IMAGE top
+    run_podman --events-backend=file run -d --name mycontainer $IMAGE top
     run_podman pod create mypod
     run_podman secret create mysecret /etc/hosts
     if is_rootless; then
@@ -150,8 +150,10 @@ function check_subcommand() {                                                                                                             
     run_podman pod rm mypod
     run_podman rmi $(pause_image)
     run_podman rm -f -t0 mycontainer
-    run_podman machine rm -f mymachine
     run_podman secret rm mysecret
+    if is_rootless; then
+        run_podman machine rm -f mymachine
+    fi
 
     # Make sure there are no leftover commands in our table - this would
     # indicate a typo in the table, or a flaw in our logic such that

Luap99 · 2022-09-12T12:32:39Z

We can wait for #15717 to avoid the hang, instead of the awkward workaround:
run_podman --events-backend=file run -d --name mycontainer $IMAGE top

edsantiago · 2022-09-12T12:33:46Z

SGTM but you still need the rootless fixes because I screwed up in my earlier version.

Currently the podman command --format output code uses a mix of report.Formatter and report.Template. I patched report.Formatter to correctly handle newlines[1]. Since we cannot fix this with report.Template we have to migrate all users to report.Formatter. This ensures consistent behavior for all commands. This change does not change the output, we can add a new test for the newline bug when the common PR is vendored in. [1] containers/common#1146 Signed-off-by: Paul Holzinger <pholzing@redhat.com>

Currently the podman command --format output code uses a mix of report.Formatter and report.Template. I patched report.Formatter to correctly handle newlines[1]. Since we cannot fix this with report.Template we have to migrate all users to report.Formatter. This ensures consistent behavior for all commands. This change does not change the output, we can add a new test for the newline bug when the common PR is vendored in. Also fix a bug where a invlaid template would not cause a exit code > 0, see the added test case. [1] containers/common#1146 Signed-off-by: Paul Holzinger <pholzing@redhat.com>

Currently the podman command --format output code uses a mix of report.Formatter and report.Template. I patched report.Formatter to correctly handle newlines[1]. Since we cannot fix this with report.Template we have to migrate all users to report.Formatter. This ensures consistent behavior for all commands. This change does not change the output, we can add a new test for the newline bug when the common PR is vendored in. Also fixa bug since the table format is expected to print headers as well. [1] containers/common#1146 Signed-off-by: Paul Holzinger <pholzing@redhat.com>

Currently the podman command --format output code uses a mix of report.Formatter and report.Template. I patched report.Formatter to correctly handle newlines[1]. Since we cannot fix this with report.Template we have to migrate all users to report.Formatter. This ensures consistent behavior for all commands. This change does not change the output, we can add a new test for the newline bug when the common PR is vendored in. [1] containers/common#1146 Signed-off-by: Paul Holzinger <pholzing@redhat.com>

Unlikely to happen but when there is an error printing the data to stdout (either as json or go template) we should not just log it and exit with 0. Instead return a proper error and exit with 125. Signed-off-by: Paul Holzinger <pholzing@redhat.com>

Signed-off-by: Ed Santiago <santiago@redhat.com>

Currently the podman command --format output code uses a mix of report.Formatter and report.Template. I patched report.Formatter to correctly handle newlines[1]. Since we cannot fix this with report.Template we have to migrate all users to report.Formatter. This ensures consistent behavior for all commands. This change does not change the output. [1] containers/common#1146 Signed-off-by: Paul Holzinger <pholzing@redhat.com>

Now that commit d10e77e is merged, it will reuse the same template logic as inspect and therefore should just work. Also remove the FIXME from eds test. Signed-off-by: Paul Holzinger <pholzing@redhat.com>

edsantiago · 2022-09-13T11:09:50Z

test/system/610-format.bats

+    run_podman pod rm mypod
+    run_podman rmi $(pause_image)
+    run_podman rm -f -t0 mycontainer
+    run_podman machine rm -f mymachine


Need to kill this line

edsantiago · 2022-09-13T11:13:20Z

I encourage you to test, it only takes a few seconds to run:

$ hack/bats format

Luap99 · 2022-09-13T11:21:53Z

@edsantiago It still fails as root with:

   #/vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
   #|     FAIL: Did not find (or test) subcommands:
   #| expected: = ''
   #|   actual:   'podman machine inspect'

The problem is we do not remove the machine command from the test table as root.

edsantiago · 2022-09-13T11:32:22Z

Guess I should test too -- my apologies. Please apply the following workaround, I will work on something cleaner later:

diff --git a/test/system/610-format.bats b/test/system/610-format.bats
index bedfab47f..1c16db84e 100644
--- a/test/system/610-format.bats
+++ b/test/system/610-format.bats
@@ -53,6 +53,7 @@ function check_subcommand() {                                                                                                                
         # Special case: 'podman machine' can't be run as root. No override.
         if [[ "$cmd" = "machine" ]]; then
             if ! is_rootless; then
+                unset extra_args["podman machine inspect"]
                 continue
             fi
         fi

edsantiago · 2022-09-13T13:45:33Z

Well, this was unexpected:

Error: exec: "qemu-system-x86_64": executable file not found in $PATH

(in ubuntu

I need to do some cleanup on this test, but it's not fair to keep this PR hanging on that. For now just add skip_if_ubuntu. I'm really sorry that this has been so painful.

Luap99 · 2022-09-13T14:41:33Z

Well, this was unexpected:
Error: exec: "qemu-system-x86_64": executable file not found in $PATH
(in ubuntu

I need to do some cleanup on this test, but it's not fair to keep this PR hanging on that. For now just add skip_if_ubuntu. I'm really sorry that this has been so painful.

No worries, we suffer together :)

This version does a much better job of error reporting and also catches more commands. Changes from edsantiago. Signed-off-by: Paul Holzinger <pholzing@redhat.com>

edsantiago · 2022-09-13T18:10:10Z

Whew!
//lgtm

edsantiago · 2022-09-13T18:10:20Z

/lgtm

Followup to containers#15673 (--format with newlines). I cobbled up a test for it, but I was sloppy, so the test had issues that I kept having to band-aid. This is a cleaner way to handle podman-machine. Signed-off-by: Ed Santiago <santiago@redhat.com>

Followup to containers#15673 (--format with newlines). I cobbled up a test for it, but I was sloppy, so the test had issues that I kept having to band-aid. This is a cleaner way to handle podman-machine. ...and, another unexpected surprise with podman stats. It fails under rootless cgroupsv1. We can't sweep it under the rug via skip_if_ubuntu because tests will then fail on RHEL8. So, add a similar mechanism for testing podman stats. Signed-off-by: Ed Santiago <santiago@redhat.com>

Followup to containers#15673 (--format with newlines). I cobbled up a test for it, but I was sloppy, so the test had issues that I kept having to band-aid. This is a cleaner way to handle podman-machine. ...and, another unexpected surprise with podman stats. It fails under rootless cgroupsv1. We can't sweep it under the rug via skip_if_ubuntu because tests will then fail on RHEL8. So, add a similar mechanism for testing podman stats. ...plus a non-surprise, the 'search' test flakes. Try minimizing that by searching only $IMAGE. If quay.io is down, other tests will certainly fail. Signed-off-by: Ed Santiago <santiago@redhat.com>

openshift-ci bot added the release-note label Sep 7, 2022

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

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 7, 2022

Luap99 force-pushed the template branch from cce4b5d to fd09fa6 Compare September 8, 2022 08:58

Luap99 mentioned this pull request Sep 8, 2022

podman events missbehaving when there are zero events #15688

Closed

Luap99 force-pushed the template branch from bdcf08a to bc727c3 Compare September 8, 2022 15:53

jwhonce reviewed Sep 8, 2022

View reviewed changes

Luap99 force-pushed the template branch 5 times, most recently from 7aa2fa2 to 7933f15 Compare September 9, 2022 13:51

Luap99 force-pushed the template branch from 7933f15 to 34126cb Compare September 12, 2022 10:13

Luap99 added 6 commits September 13, 2022 10:33

Luap99 and others added 6 commits September 13, 2022 10:33

tests for 15673

1acebdd

Signed-off-by: Ed Santiago <santiago@redhat.com>

enable podman pod inspect --format test

908458d

Now that commit d10e77e is merged, it will reuse the same template logic as inspect and therefore should just work. Also remove the FIXME from eds test. Signed-off-by: Paul Holzinger <pholzing@redhat.com>

Luap99 force-pushed the template branch from 34126cb to ea73502 Compare September 13, 2022 08:37

edsantiago reviewed Sep 13, 2022

View reviewed changes

Luap99 force-pushed the template branch from ea73502 to 43e1d96 Compare September 13, 2022 11:35

rework --format system test

9d41b95

This version does a much better job of error reporting and also catches more commands. Changes from edsantiago. Signed-off-by: Paul Holzinger <pholzing@redhat.com>

Luap99 force-pushed the template branch from 43e1d96 to 9d41b95 Compare September 13, 2022 15:02

openshift-ci bot assigned edsantiago Sep 13, 2022

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

edsantiago removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 13, 2022

openshift-merge-robot merged commit ad529f3 into containers:main Sep 13, 2022

Luap99 deleted the template branch September 13, 2022 18:34

edsantiago mentioned this pull request Sep 13, 2022

System tests: cleanup in --format test #15786

Merged

cblecker mentioned this pull request Feb 23, 2023

Use common library reporter containers/skopeo#1822

Merged

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix go template parsing with "\n" in it #15673

Fix go template parsing with "\n" in it #15673

Luap99 commented Sep 7, 2022

openshift-ci bot commented Sep 7, 2022

Luap99 commented Sep 7, 2022

mheon commented Sep 7, 2022

edsantiago commented Sep 7, 2022

edsantiago commented Sep 7, 2022

edsantiago commented Sep 8, 2022

jwhonce Sep 8, 2022

Luap99 Sep 8, 2022

edsantiago commented Sep 9, 2022

Luap99 commented Sep 12, 2022

edsantiago commented Sep 12, 2022

Luap99 commented Sep 12, 2022

edsantiago commented Sep 12, 2022

edsantiago Sep 13, 2022

edsantiago commented Sep 13, 2022

Luap99 commented Sep 13, 2022

edsantiago commented Sep 13, 2022

edsantiago commented Sep 13, 2022

Luap99 commented Sep 13, 2022

edsantiago commented Sep 13, 2022

edsantiago commented Sep 13, 2022

Fix go template parsing with "\n" in it #15673

Fix go template parsing with "\n" in it #15673

Conversation

Luap99 commented Sep 7, 2022

Does this PR introduce a user-facing change?

openshift-ci bot commented Sep 7, 2022

Luap99 commented Sep 7, 2022

mheon commented Sep 7, 2022

edsantiago commented Sep 7, 2022

edsantiago commented Sep 7, 2022

edsantiago commented Sep 8, 2022

jwhonce Sep 8, 2022

Choose a reason for hiding this comment

Luap99 Sep 8, 2022

Choose a reason for hiding this comment

edsantiago commented Sep 9, 2022

Luap99 commented Sep 12, 2022

edsantiago commented Sep 12, 2022

Luap99 commented Sep 12, 2022

edsantiago commented Sep 12, 2022

edsantiago Sep 13, 2022

Choose a reason for hiding this comment

edsantiago commented Sep 13, 2022

Luap99 commented Sep 13, 2022

edsantiago commented Sep 13, 2022

edsantiago commented Sep 13, 2022

Luap99 commented Sep 13, 2022

edsantiago commented Sep 13, 2022

edsantiago commented Sep 13, 2022