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

Enable logs upload for agama playwright tests #17674

Merged
merged 1 commit into from Sep 1, 2023

Conversation

manfredi
Copy link
Contributor

@rakoenig
Copy link
Contributor

LGTM, so we have a tarball with all the traces.zip in their individual directories in it. Saves us from some headaches when we design tests.

@jknphy
Copy link
Contributor

jknphy commented Aug 30, 2023

Looks good, please move all this code to agama_base. I can see that it needs to happen before the reboot, so we need some test module in perl that does that, is there any existing one, if not, we can create it. reboot should not be part of the agama process as we can trigger it in different ways actually.

@manfredi manfredi force-pushed the issues-134690 branch 17 times, most recently from c6756d2 to 3e17a5b Compare August 31, 2023 10:34
@manfredi
Copy link
Contributor Author

Code moved inside lib/Yam/agama/agama_base.pm

lib/Yam/agama/agama_base.pm Outdated Show resolved Hide resolved
jknphy
jknphy previously approved these changes Aug 31, 2023
@@ -35,4 +36,9 @@ sub test_flags {
return {fatal => 1};
}

sub upload_traces {
assert_script_run("tar czf /tmp/traces.tar.gz test-results/");
Copy link
Contributor

@jknphy jknphy Aug 31, 2023

Choose a reason for hiding this comment

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

could you check why this doesn't work for you?
tar_and_upload_log()
actually it would be better not to use assert_script_run and use script_run, otherwise you never get logs of other thing you try to get log below this point, the function on top probably do not use either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Inside the ISO it seems that bzip2 is not present: bzip2: command not found.
The sub tar_and_upload_log() use it to compress the tarball by using the options tar -jcv -f.

Modified the sub upload_traces(), so that continue in case of failure.

Copy link
Contributor

@jknphy jknphy Sep 1, 2023

Choose a reason for hiding this comment

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

we use for yast logs get_available_compression(), it happens in minimal system the same thing. It is quite hard to map a linux command with a wrapper, you never get exactly what you want if you just map its options "a little"like in this case, and if you map it fully, probably doesn't worth the effort for the given use, and it would better to do it with some cpan module, so it is kind doing bash in Perl what we are doing in the repo IMO.

We could update that function passing in $args the compression, but that would be just a letter "j" or "z" and that would not be very readable, as there are not enums in perl, we would need constants or passing the whole word (which something there isn't depending on the command arg of a given command) but you can add a comment in the documentation of the function describing both formats and that would help, it would be something like $args->{compression_options} which would take a default value of "j" to not break existing code.
what solution you prefer? we can go both ways.

@jknphy jknphy dismissed their stale review August 31, 2023 12:35

dismissing review due to assert_script might avoid log recollection

@jknphy jknphy merged commit 59025c4 into os-autoinst:master Sep 1, 2023
7 checks passed
@manfredi manfredi deleted the issues-134690 branch September 1, 2023 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants