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

buildiso: Introduce exclude-systems CLI option and distro autodetection #3665

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

aaannz
Copy link
Contributor

@aaannz aaannz commented Mar 19, 2024

Linked Items

Fixes #

Description

This commit makes --distro option optional, if not provided first profile or first system is used to get the distro automatically.

This commit introduces --exclude-systems option. This instructs buildiso to not write any system records entries to the isolinux/grub config. Useful particularly when only profile entries are requested.

Behaviour changes

Old: buildiso --distro is mandatory option. There is no way to export config only for specific profiles if systems exists too.

New: --distro is optional, determined from first profile or system if not set. Use --exclude-systems to export configuration only for provided profiles.

Category

This is related to a:

  • Bugfix
  • Feature
  • Packaging
  • Docs
  • Code Quality
  • Refactoring
  • Miscellaneous

Tests

  • Unit-Tests were created
  • System-Tests were created
  • Code is already covered by Unit-Tests
  • Code is already covered by System-Tests
  • No tests required

This commit makes --distro option optional, if not provided first profile
or first system is used to get the distro automatically.

This commit introduces --exclude-systems option. This instructs buildiso
to not write any system records entries to the isolinux/grub config.
Useful particularly when only profile entries are requested.
@m-czernek m-czernek requested a review from a team March 19, 2024 16:44
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.

The code itself looks OK to me, but we need to fix some of the CI fails:

  • Add Black formatting
  • Add the missing changelog entry
  • Import error that causes the tests to fail (I think that's the same issue as some of the Codacy failures, together with some missing type declaration(s))

That should fix a lot of the CI fails

Copy link
Contributor

@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.

Changes for netboot LGTM!

Could you also add --exclude-systems for buildiso.standalone?

@aaannz
Copy link
Contributor Author

aaannz commented Mar 21, 2024

About the imports, I see that Distro was there before and is under TYPE_CHECKING condition. So I assume we do not want to have to import Profile and System by default.

Am I correct to instead of e.g. List[Profile] add like List["Profile"] and add new Optional where needed?

Copy link
Member

@SchoolGuy SchoolGuy left a comment

Choose a reason for hiding this comment

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

Thank you for this addition. I would argue that the Git commit log could be cleaned up but it is not mandatory in my eyes to merge this PR.

@SchoolGuy SchoolGuy requested a review from agraul April 16, 2024 08:46
@agraul
Copy link
Contributor

agraul commented Apr 22, 2024

My question still stands, is this something that is also needed for --standalone?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

None yet

4 participants