Update action version to 4 #33

b10n1k · 2024-02-09T09:09:54Z

No description provided.

baierjan · 2024-02-09T09:17:10Z

.github/workflows/isotovideo-action.yml


      - name: fail if any test module failed
-        run: jq .result testresults/result-*.json | grep -v ok && echo "Test modules failed" && exit 1
+        run: jq .result testresults/result-*.json | grep ok || (echo "Test modules failed" && exit 1)


Consider #31 (comment)

Suggested change

run: jq .result testresults/result-*.json | grep ok || (echo "Test modules failed" && exit 1)

run: jq .result testresults/result-*.json | grep -v ok && { echo "Test modules failed" && exit 1; } || true

Please see #33 (comment)

baierjan · 2024-02-09T09:19:43Z

.github/workflows/isotovideo-check-all-test-modules.yml

+          && jq .result testresults/result-*.json | grep ok \
+          || (echo "Test modules failed" && exit 1)'


Same grep and subshell problem as above,

Suggested change

&& jq .result testresults/result-*.json | grep ok \

|| (echo "Test modules failed" && exit 1)'

&& jq .result testresults/result-*.json | grep -v ok \

&& { echo "Test modules failed" && exit 1; } || true'

baierjan · 2024-02-09T09:20:35Z

.github/workflows/isotovideo-check-all-test-modules.yml

@@ -13,4 +13,9 @@ jobs:
    steps:
      - uses: actions/checkout@v2
      - name: Run isotovideo against test code, fail if any test module failed
-        run: podman run --rm -it -v .:/tests:Z --entrypoint '' registry.opensuse.org/devel/openqa/containers/isotovideo:qemu-x86-jq /bin/sh -c 'isotovideo qemu_no_kvm=1 casedir=/tests && jq .result testresults/result-*.json | grep -v ok && echo "Test modules failed" && exit 1'
+        run: |
+          docker run --rm -v .:/tests:Z --entrypoint '' \


Consider #31 (comment)

Is the switch from podman to docker intended? I thought podman is the preferred way.

podman is not provided in the container. I found it easier to just switch to docker. i am not sure if that matters here

I did not observe that. See https://github.com/os-autoinst/os-autoinst-distri-example/actions/runs/7275772852/job/19825501448#step:3:1

I also saw that we still run ubuntu-20.04 here, created

CI: Use ubuntu-latest again after 18.04 network problems are solved #34

Whops, I will close #35 in favor of yours, it seems we observed the same. 😄

okurz · 2024-02-09T09:17:52Z

.github/workflows/isotovideo-action.yml


      - name: Run isotovideo against test code
-        run: isotovideo qemu_no_kvm=1 casedir=.
+        run: isotovideo qemu_no_kvm=1 casedir=$(pwd)


why not keep .?

not a significant reason. personal preference. it s quite similar

then please revert

okurz · 2024-02-09T09:21:45Z

.github/workflows/isotovideo-action.yml


      - name: fail if any test module failed
-        run: jq .result testresults/result-*.json | grep -v ok && echo "Test modules failed" && exit 1
+        run: jq .result testresults/result-*.json | grep ok || (echo "Test modules failed" && exit 1)


that grep ok would just mean that there is at least one test module that failed. But the purpose was to fail if any module fails. As we expect the execution in the example to fail what we can do is

Suggested change

run: jq .result testresults/result-*.json | grep ok || (echo "Test modules failed" && exit 1)

run: jq .result testresults/result-*.json | grep -v ok && (echo "At least one test module failed as expected" && exit 0)

i applied suggestion from @baierjan. review and resolve please

Yes, I saw that you applied the suggestion from @baierjan. The problem is that this way we are ignoring results either way. But we should check for the expected result which is actually that test modules should fail which serves as the example

Do you suggest that it is expected that the test will fail (i.e. the result of the isotovideo is no ok)? Running the pipeline locally yields testresults/result-boot.json with the "result" : "ok", so the original code

os-autoinst-distri-example/.github/workflows/isotovideo-action.yml

Line 19 in 10683a4

run: jq .result testresults/result-*.json | grep -v ok && echo "Test modules failed" && exit 1

only fails the pipeline because of the empty match from grep (i.e. after removing all ok results, there is no other record).

hm, you are right. That changed in a pull request from @Martchus some months ago. Then it should be

run: jq .result testresults/result-*.json | grep -v ok || (echo "Test modules failed" && exit 1)

right?

I believe not. The success of grep -v ok means something else than ok was matched (and that should be a failure); on the other hand, if grep -v ok fails (with exit code 1, btw.) it means there is no other result than ok, so it is actually a success.
So what we want to have is

if $(jq .result testresults/result-*.json | grep -v ok); then echo "Test modules failed"; exit 1; else exit 0; fi

My original suggestion jq .result testresults/result-*.json | grep -v ok && { echo "Test modules failed" && exit 1; } || true should be the same. The "only" issue here is, that it masks error in jq. If we want to solve that, we need to introduce a little more logic.

Maybe

test $(jq .result testresults/result-*.json | grep -v ok | wc -l) -eq 0 || { echo "Test modules failed"; exit 1; }

could be more readable?

I have an even better idea as @josegomezr already fixed that use case for us :) -> #36

Yes, that is the perfect solution.

okurz · 2024-02-09T09:21:56Z

.github/workflows/isotovideo-check-all-test-modules.yml

+          registry.opensuse.org/devel/openqa/containers/isotovideo:qemu-x86-jq /bin/sh \
+          -c 'isotovideo qemu_no_kvm=1 casedir=/tests \
+          && jq .result testresults/result-*.json | grep ok \
+          || (echo "Test modules failed" && exit 1)'


same as above

i applied suggestion from @baierjan. review and resolve please

okurz · 2024-02-09T09:22:08Z

.github/workflows/isotovideo.yml

-        run: podman run --rm -it -v .:/tests:Z registry.opensuse.org/devel/openqa/containers/isotovideo:qemu-x86 qemu_no_kvm=1 casedir=/tests
+        run: docker run --rm -v .:/tests:Z registry.opensuse.org/devel/openqa/containers/isotovideo:qemu-x86 qemu_no_kvm=1 casedir=/tests


Why would you change that?

podman is not provided in the container. I found it easier to just switch to docker.

In which container? The ubuntu-latest provided by Github includes podman version 3.4.4 (see https://github.com/baierjan/actions-experiments/actions/runs/7842150230/job/21399883954)

baierjan · 2024-02-09T09:33:15Z

.github/workflows/isotovideo-check-all-test-modules.yml

@@ -13,4 +13,9 @@ jobs:
    steps:
      - uses: actions/checkout@v2


#31 (comment)

Can you also change the action to actions/checkout@v4?

Sure. i was thinking about that. I just do not know where to find the latest version

https://github.com/actions/checkout

While reviewing #33 I found out that the comment about latest images is no longer relevant and in fact the latest image is now pointing to version 22.04. Let's remove the override and get back to latest image.

.github/workflows/isotovideo-action.yml

baierjan

I believe we want to rebase over #36 and see what is left here.

b10n1k · 2024-02-09T22:45:52Z

I believe we want to rebase over #36 and see what is left here.

Do we even need it anymore?

okurz · 2024-02-10T21:12:10Z

@b10n1k I incorporated your great ideas to fix the CI jobs but at least c3ab292 would be beneficial on top

baierjan · 2024-02-12T08:22:38Z

@b10n1k I incorporated your great ideas to fix the CI jobs but at least c3ab292 would be beneficial on top

Yes, also the action version change to mitigate the node.js deprecation warning would be nice.

baierjan

Only a minor issue, the commit message now does not correspond with the changes inside the commit; otherwise LGTM

LGTM

okurz

Correct, "8428444" shouldn't say that it would fix isotovideo CI jobs

Signed-off-by: ybonatakis <[email protected]>

Due to isotovideo:qemu-x86-jq the step which installs jq is not needed. Signed-off-by: ybonatakis <[email protected]>

b10n1k requested review from baierjan and okurz February 9, 2024 09:11

baierjan requested changes Feb 9, 2024

View reviewed changes

okurz requested changes Feb 9, 2024

View reviewed changes

baierjan reviewed Feb 9, 2024

View reviewed changes

b10n1k force-pushed the fix_pipeline_isotovideo branch from 51bd33e to b0b99d5 Compare February 9, 2024 09:43

baierjan mentioned this pull request Feb 9, 2024

Update ubuntu image to latest #35

Closed

b10n1k mentioned this pull request Feb 9, 2024

CI: Use ubuntu-latest again after 18.04 network problems are solved #34

Merged

b10n1k force-pushed the fix_pipeline_isotovideo branch from b0b99d5 to 8e6ab39 Compare February 9, 2024 09:56

okurz reviewed Feb 9, 2024

View reviewed changes

.github/workflows/isotovideo-action.yml Show resolved Hide resolved

b10n1k force-pushed the fix_pipeline_isotovideo branch from 8e6ab39 to c3ab292 Compare February 9, 2024 13:32

b10n1k requested review from okurz and baierjan February 9, 2024 14:31

baierjan previously requested changes Feb 9, 2024

View reviewed changes

b10n1k force-pushed the fix_pipeline_isotovideo branch from c3ab292 to c531d10 Compare February 12, 2024 13:11

b10n1k requested a review from baierjan February 12, 2024 13:11

okurz approved these changes Feb 12, 2024

View reviewed changes

baierjan reviewed Feb 12, 2024

View reviewed changes

okurz requested changes Feb 12, 2024

View reviewed changes

b10n1k added 2 commits February 12, 2024 14:58

Update action version to 4

4531e03

Signed-off-by: ybonatakis <[email protected]>

Remove unnecessary step from isotovideo-action.yml

316737b

Due to isotovideo:qemu-x86-jq the step which installs jq is not needed. Signed-off-by: ybonatakis <[email protected]>

b10n1k force-pushed the fix_pipeline_isotovideo branch from c531d10 to 316737b Compare February 12, 2024 13:59

b10n1k changed the title ~~Fix isotovideo CI jobs~~ Update action version to 4 Feb 12, 2024

b10n1k requested review from baierjan and okurz February 12, 2024 13:59

okurz approved these changes Feb 12, 2024

View reviewed changes

baierjan approved these changes Feb 12, 2024

View reviewed changes

okurz merged commit d2c0b08 into os-autoinst:main Feb 12, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update action version to 4 #33

Update action version to 4 #33

b10n1k commented Feb 9, 2024 •

edited

Loading

baierjan Feb 9, 2024

okurz Feb 9, 2024

baierjan Feb 9, 2024

baierjan Feb 9, 2024

b10n1k Feb 9, 2024

okurz Feb 9, 2024

okurz Feb 9, 2024

baierjan Feb 9, 2024

okurz Feb 9, 2024

b10n1k Feb 9, 2024

okurz Feb 9, 2024

okurz Feb 9, 2024

b10n1k Feb 9, 2024

okurz Feb 9, 2024

baierjan Feb 9, 2024

okurz Feb 9, 2024

baierjan Feb 9, 2024

okurz Feb 9, 2024

baierjan Feb 9, 2024

okurz Feb 9, 2024

b10n1k Feb 9, 2024

okurz Feb 9, 2024

b10n1k Feb 9, 2024

baierjan Feb 9, 2024

baierjan Feb 9, 2024

b10n1k Feb 9, 2024

baierjan Feb 9, 2024

b10n1k Feb 9, 2024

baierjan left a comment

b10n1k commented Feb 9, 2024

okurz commented Feb 10, 2024

baierjan commented Feb 12, 2024

baierjan left a comment

okurz left a comment

	run: jq .result testresults/result-*.json \| grep ok \|\| (echo "Test modules failed" && exit 1)
	run: jq .result testresults/result-*.json \| grep -v ok && { echo "Test modules failed" && exit 1; } \|\| true

		&& jq .result testresults/result-*.json \| grep ok \
		\|\| (echo "Test modules failed" && exit 1)'

		run: podman run --rm -it -v .:/tests:Z registry.opensuse.org/devel/openqa/containers/isotovideo:qemu-x86 qemu_no_kvm=1 casedir=/tests
		run: docker run --rm -v .:/tests:Z registry.opensuse.org/devel/openqa/containers/isotovideo:qemu-x86 qemu_no_kvm=1 casedir=/tests

Update action version to 4 #33

Update action version to 4 #33

Conversation

b10n1k commented Feb 9, 2024 • edited Loading

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

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

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

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

baierjan left a comment

Choose a reason for hiding this comment

b10n1k commented Feb 9, 2024

okurz commented Feb 10, 2024

baierjan commented Feb 12, 2024

baierjan left a comment

Choose a reason for hiding this comment

okurz left a comment

Choose a reason for hiding this comment

b10n1k commented Feb 9, 2024 •

edited

Loading