Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Align CI workflows from os-autoinst-common and isotovideo #28

Merged
merged 1 commit into from Dec 20, 2023

Conversation

josegomezr
Copy link
Contributor

@josegomezr josegomezr commented Dec 14, 2023

  • Pulled commit message checker from os-autoinst-commons

Complements os-autoinst/os-autoinst-commons#30

poo#138416

.github/workflows/openqa.yml Outdated Show resolved Hide resolved
scripts/validate-test-results Outdated Show resolved Hide resolved
scripts/validate-test-results Outdated Show resolved Hide resolved
scripts/validate-test-results Outdated Show resolved Hide resolved
Copy link
Contributor

@Martchus Martchus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See inline-comments of my previous review

my @test_results_to_check = ();

while(@args){
my $arg = shift @args;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no tabs please :)
Isn't this file linted itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Next change after perltidy on os-autoinst-commons is a generic perltidy & perl critic workflow that can be used like the git commit checker.

as per my experiments here: https://github.com/josegomezr/perl-toolkit

tests/boot.pm Outdated Show resolved Hide resolved
Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Next to the already stated comment there is also the git commit message which does not adhere to the style that the git commit message checker would actually check. After you fixed all comments an the git commit message I will need to take another look so there might be more open points.

@josegomezr
Copy link
Contributor Author

the git commit message which does not adhere to the style that the git commit message checker would actually check.

Where? What's missing?

@josegomezr josegomezr force-pushed the simplify_example_pipeline branch 2 times, most recently from 8042abb to 764f064 Compare December 15, 2023 14:40
@josegomezr josegomezr force-pushed the simplify_example_pipeline branch 2 times, most recently from 1110758 to 7aced50 Compare December 15, 2023 15:05
@okurz
Copy link
Member

okurz commented Dec 17, 2023

Phrase git commit message subject lines in imperative mood, e.g. s/Linting & Improved workflows/Add CI workflows from os-autoinst-common/ or something like that

@josegomezr josegomezr changed the title Improved example workflows & small lint fixes. Align CI workflows from os-autoinst-common and isotovideo Dec 18, 2023
@josegomezr josegomezr force-pushed the simplify_example_pipeline branch 2 times, most recently from e7fb8ac to 5949005 Compare December 19, 2023 15:47
Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand how the changes align to

Regarding the actual changes I don't think it's a wise idea to build in that much reliance on github actions. As much as possible I would like us to stay independant of the actual CI use and keep commands that we call in there as simple as possible. That's why I prefer a command like

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

to the rather verbose extended results you do.

Please also see my inline comments.

scripts/validate-test-results Outdated Show resolved Hide resolved
scripts/validate-test-results Outdated Show resolved Hide resolved
main.pm Outdated Show resolved Hide resolved
@josegomezr
Copy link
Contributor Author

That's why I prefer a command like [...]

Nothing prevents you from that. it's written on "GitHub Actions" lang sort-of-speak. But you can get to do the podman run, and that invocation style should be documented where isotovideo is built & it's images are created.

@okurz
Copy link
Member

okurz commented Dec 20, 2023

That's why I prefer a command like [...]

Nothing prevents you from that. it's written on "GitHub Actions" lang sort-of-speak. But you can get to do the podman run, and that invocation style should be documented where isotovideo is built & it's images are created.

You have to keep in mind that os-autoinst-distri-example is used as example as well as template for new users and new test distributions. Also see what https://github.com/os-autoinst/os-autoinst-distri-example/blob/main/README.md tells about that.

Regarding the CI workflow please also see http://open.qa/docs/#_create_and_monitor_openqa_jobs_from_within_the_ci_runner where we reference the github workflow definitions from os-autoinst-distri-example

- Brought the commit message checker workflow from os-autoinst-commons.
@josegomezr
Copy link
Contributor Author

As discussed today, Bringing the commit message from commons only.

@josegomezr
Copy link
Contributor Author

Addenda:

#29 has the github-native workflows for isotovideo.
#30 has the strict/warnings/Mojo::Base changes.

Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commit message check looks good

@okurz okurz merged commit 10683a4 into os-autoinst:main Dec 20, 2023
6 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants