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

Get list of installer image packages when probing boot.iso #1229

Conversation

rvykydal
Copy link
Contributor

@rvykydal rvykydal commented Jun 25, 2024

You can see the change in action in the /test-os-variants workflow of this PR, step "Run kickstart tests in container" (near "Saving list of installer image packages to /var/tmp/kstest-image-packages.log")
or in logs / artefacts of the workflow (eg logs-daily-iso) where you can find kstest-image-packages.log.

@rvykydal rvykydal marked this pull request as draft June 25, 2024 10:49
@rvykydal
Copy link
Contributor Author

/test-os-variants

@rvykydal rvykydal marked this pull request as ready for review June 25, 2024 12:10
@rvykydal rvykydal force-pushed the gather-list-of-packages-when-probing-boot-iso branch from 006ad59 to 31837f4 Compare June 25, 2024 13:11
@rvykydal
Copy link
Contributor Author

/test-os-variants

Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

Very useful thanks.

Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just a question below.

ISO_PACKAGES=$(echo "${output}" | grep 'PACKAGES=')
ISO_PACKAGES="${ISO_PACKAGES##PACKAGES=}"
echo Saving list of installer image packages to /var/tmp/kstest-image-packages.log
echo $ISO_PACKAGES | tr ' ' '\n' | tee /var/tmp/kstest-image-packages.log
Copy link
Member

Choose a reason for hiding this comment

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

If I read this correctly then the list of packages will get printed to stdout. If this is correct I think it would spam the test results a lot wouldn't it? Isn't it enough to just store the list?

Copy link
Contributor

Choose a reason for hiding this comment

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

If the tests pass, you do not need to worry about extra lines in the output, if there is a failure it might be handy to see the package versions right in the test output. I actually like it this way :-)

Copy link
Contributor Author

@rvykydal rvykydal Jun 26, 2024

Choose a reason for hiding this comment

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

I think in this case it is handy to have this in the main log output without need of making sure we really pass / gather all the required logs (for example in GH actions). Also it is maybe more discoverable this way. Actually this information is very important when dealing with integration tests failures so let's make it as easily accessible as possible.

@jkonecny12
Copy link
Member

Yes, it seems to me that it creates a mess in GH actions console output:
https://github.com/rhinstaller/kickstart-tests/actions/runs/9663125740/job/26654555035#step:15:107

I think we should remove use of tee to keep the log clarity.

ISO_PACKAGES=$(echo "${output}" | grep 'PACKAGES=')
ISO_PACKAGES="${ISO_PACKAGES##PACKAGES=}"
echo Saving list of installer image packages to /var/tmp/kstest-image-packages.log
echo $ISO_PACKAGES | tr ' ' '\n' | tee /var/tmp/kstest-image-packages.log
Copy link
Contributor

Choose a reason for hiding this comment

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

If the tests pass, you do not need to worry about extra lines in the output, if there is a failure it might be handy to see the package versions right in the test output. I actually like it this way :-)

@rvykydal
Copy link
Contributor Author

Yes, it seems to me that it creates a mess in GH actions console output: https://github.com/rhinstaller/kickstart-tests/actions/runs/9663125740/job/26654555035#step:15:107

I think we should remove use of tee to keep the log clarity.

I already replied earlier, also given we even already have the debugging shell output above I don't see it as a problem.

@rvykydal rvykydal merged commit 4ffdb45 into rhinstaller:master Jun 27, 2024
5 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.

4 participants