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

Recognize squashfs build results from kiwi #7685

Merged
merged 1 commit into from
Mar 19, 2024

Conversation

aaannz
Copy link
Contributor

@aaannz aaannz commented Oct 13, 2023

What does this PR change?

Our kiwi build inspection was missing squashfs output format. This PR adds recognition of it.

GUI diff

No difference.

  • DONE

Documentation

  • No documentation needed: only internal and user invisible changes

  • DONE

Test coverage

  • No tests: integration tests cover for regressions, no unit tests are present

  • DONE

Links

Fixes https://github.com/SUSE/spacewalk/issues/22772
Tracks https://github.com/SUSE/spacewalk/pull/22743

  • DONE

Changelogs

Make sure the changelogs entries you are adding are compliant with https://github.com/uyuni-project/uyuni/wiki/Contributing#changelogs and https://github.com/uyuni-project/uyuni/wiki/Contributing#uyuni-projectuyuni-repository

If you don't need a changelog check, please mark this checkbox:

  • No changelog needed

If you uncheck the checkbox after the PR is created, you will need to re-run changelog_test (see below)

Re-run a test

If you need to re-run a test, please mark the related checkbox, it will be unchecked automatically once it has re-run:

  • Re-run test "changelog_test"
  • Re-run test "backend_unittests_pgsql"
  • Re-run test "java_pgsql_tests"
  • Re-run test "schema_migration_test_pgsql"
  • Re-run test "susemanager_unittests"
  • Re-run test "javascript_lint"
  • Re-run test "spacecmd_unittests"

@aaannz aaannz requested a review from a team as a code owner October 13, 2023 11:22
@aaannz aaannz requested review from agraul and removed request for a team October 13, 2023 11:22
Copy link
Member

@agraul agraul left a comment

Choose a reason for hiding this comment

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

The change itself looks good, but I'm questioning the approach of having to a list of known suffixes. Not something we need to fix in this PR, but maybe we can think about it.

I noticed you've put "No tests: already covered" in the PR description. Can you point me to the test case that covers squashfs build results? I'm a bit surprised this is already covered since the changelog entry mentions a bug report.

Please keep in mind that merging this PR will close the Bugzilla-tracking GH issue. You might need to re-open it.

@aaannz
Copy link
Contributor Author

aaannz commented Oct 13, 2023

I don't know about correct key how to select the end image result from the kiwi build. File size is not correct as you can have compressed images which are smaller then original image also present in the build results directory. So we opted for suffixes list and instead of inspecting all possible kiwi results we are doing this per-bug basis as customers needs.

We have tests with regards to overall regressions. It is true that we don't have any squashfs related tests. We don't have any unit tests for this particular code, only integration tests and those tests only KIS images.

@agraul
Copy link
Member

agraul commented Oct 16, 2023

I don't know about correct key how to select the end image result from the kiwi build. File size is not correct as you can have compressed images which are smaller then original image also present in the build results directory. So we opted for suffixes list and instead of inspecting all possible kiwi results we are doing this per-bug basis as customers needs.

I see. I haven't used Kiwi much, maybe this is a good way to save time then.

We have tests with regards to overall regressions. It is true that we don't have any squashfs related tests. We don't have any unit tests for this particular code, only integration tests and those tests only KIS images.

The way I see it, test coverage applies to the change and not the component that this change touches. How much work would it be to add tests for "finding the correct artifact"?

Copy link
Contributor

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Dec 16, 2023
@aaannz aaannz removed the Stale label Dec 18, 2023
@aaannz
Copy link
Contributor Author

aaannz commented Dec 18, 2023

Still valid, not enough time to focus on this.

Copy link
Contributor

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Feb 17, 2024
@aaannz aaannz force-pushed the squashfs_support branch 2 times, most recently from 73f3038 to cc24519 Compare February 26, 2024 18:10
@github-actions github-actions bot removed the Stale label Feb 27, 2024
@aaannz aaannz requested a review from m-czernek March 18, 2024 12:39
Copy link
Contributor

@m-czernek m-czernek left a comment

Choose a reason for hiding this comment

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

👍 LGTM, apologies for taking so long.

@aaannz aaannz force-pushed the squashfs_support branch from cc24519 to 86559ac Compare March 19, 2024 09:37
@aaannz aaannz merged commit 2f74f5b into uyuni-project:master Mar 19, 2024
20 checks passed
@aaannz aaannz deleted the squashfs_support branch March 19, 2024 13:36
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.

3 participants