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.

Sign up for GitHub

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

Jump to bottom

Allow dynamic plugin data classes & convert feature plugins #3424

Merged
merged 1 commit into from
Jan 15, 2025
Merged

Allow dynamic plugin data classes & convert feature plugins #3424

merged 1 commit into from
Jan 15, 2025

Conversation

happz
Copy link
Collaborator

@happz happz commented Dec 13, 2024

Plugins' _data_class is no longer the source of plugins' data classes. Instead, new get_data_class() method is added (see [1]). _data_class is not gone, it serves as the default for get_data_class(), but plugins now can provide their own implementation.

Which is exactly what prepare/feature and feature plugins do now, the plugin builds its data class from collected smaller data classes, one for each feature plugin.

There are some loose ends, namely type annorations, but the code works, both in runtime and when rendering docs.

[1] a data_class property would be nice, but the attribute must be a class-level attribute and those cannot be properties.

Pull Request Checklist

  • implement the feature
  • write the documentation

@happz happz added step | prepare Stuff related to the prepare step code | style Code style changes not affecting functionality code | no functional change "No Functional Change" intended. Patch should not change tmt's behavior in any way. labels Dec 13, 2024
@happz happz added this to the 1.41 milestone Dec 13, 2024
@happz happz force-pushed the allow-dynamic-step-data-classes branch from 6b6e9ff to 0f30185 Compare December 13, 2024 09:33
@happz happz added the ci | full test Pull request is ready for the full test execution label Dec 13, 2024
@happz happz force-pushed the allow-dynamic-step-data-classes branch from 0f30185 to 32740ee Compare December 13, 2024 09:39
@psss psss added priority | could low priority, could be included in the next release priority | should medium priority, should be included in the next release and removed priority | could low priority, could be included in the next release labels Jan 7, 2025
@happz happz force-pushed the allow-dynamic-step-data-classes branch from 32740ee to bd6a595 Compare January 9, 2025 13:55
@happz happz requested a review from martinhoyer as a code owner January 9, 2025 13:55
@falconizmi falconizmi mentioned this pull request Jan 9, 2025
Merged
1 task
@falconizmi
Copy link
Collaborator

Tested locally with WIP feature plugin buildroot https://gitlab.cee.redhat.com/baseos-qe/tmt/-/merge_requests/106 and it successfully detects it (verified in output of TMT_PLUGINS=./tmt_redhat/buildroot/ tmt run prepare --how feature --help ).
But it couldn't run the playbook of the feature plugin, this PR #3457 has temporarily workaround for it.

@falconizmi falconizmi self-requested a review January 9, 2025 14:29
@happz happz force-pushed the allow-dynamic-step-data-classes branch from bd6a595 to b317c8a Compare January 13, 2025 14:19
@happz happz modified the milestones: 1.41, 1.42 Jan 13, 2025
skycastlelily
skycastlelily approved these changes Jan 14, 2025
View reviewed changes
Copy link
Collaborator

@skycastlelily skycastlelily left a comment

Choose a reason for hiding this comment

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

LGTM^^

@happz happz added the status | ready for merge The only missing piece is to do the rebase the current 'main' and let the CI finish. label Jan 14, 2025
@happz @psss
Allow dynamic plugin data classes & convert feature plugins
6b213af 
Plugins' `_data_class` is no longer the source of plugins' data classes.
Instead, new `get_data_class()` method is added (see [1]). `_data_class`
is not gone, it serves as the default for `get_data_class()`, but
plugins now can provide their own implementation.

Which is exactly what `prepare/feature` and feature plugins do now, the
plugin builds its data class from collected smaller data classes, one
for each feature plugin.

There are some loose ends, namely type annorations, but the code works,
both in runtime and when rendering docs.

[1] a `data_class` property would be nice, but the attribute must be a
class-level attribute and those cannot be properties.
@psss psss force-pushed the allow-dynamic-step-data-classes branch from b317c8a to 6b213af Compare January 15, 2025 09:26
@happz
Copy link
Collaborator Author

happz commented Jan 15, 2025

@psss https://artifacts.osci.redhat.com/testing-farm/1f44fca6-1bba-4f99-b60e-ec6444f2a4ac/work-bootcjx6em3nm/plans/provision/bootc/execute/data/guest/default-0/tests/provision/bootc-1/output.txt looks like a "bug" in bootc plugin, could it be calling dnf while images changed and dnf has been replaced with dnf5?

@henrywang @ckyrouac ^^ any ideas?

@psss
Copy link
Collaborator

psss commented Jan 15, 2025

@psss https://artifacts.osci.redhat.com/testing-farm/1f44fca6-1bba-4f99-b60e-ec6444f2a4ac/work-bootcjx6em3nm/plans/provision/bootc/execute/data/guest/default-0/tests/provision/bootc-1/output.txt looks like a "bug" in bootc plugin, could it be calling dnf while images changed and dnf has been replaced with dnf5?

Yeah, looks like that. But I just tested locally with fresh images and everything just works nice. Which is weird.

@psss
Copy link
Collaborator

psss commented Jan 15, 2025

Anyway, this is definitely no related to this change. Merging.

@psss psss merged commit e0fc22f into main Jan 15, 2025
19 of 20 checks passed
@psss psss deleted the allow-dynamic-step-data-classes branch January 15, 2025 14:47
@psss psss self-assigned this Jan 15, 2025
@henrywang
Copy link
Contributor

henrywang commented Jan 15, 2025 •
edited
Loading

This dnf issue is caused by https://gitlab.com/fedora/bootc/base-images/-/merge_requests/77 and has been fixed by MR https://gitlab.com/fedora/bootc/base-images/-/merge_requests/78. So the new test with new fedora-bootc image is passed.

This's fedora-bootc base image issue, not bootc plugin issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@falconizmi falconizmi falconizmi approved these changes

@skycastlelily skycastlelily skycastlelily approved these changes

@psss psss psss approved these changes

@lukaszachy lukaszachy Awaiting requested review from lukaszachy lukaszachy is a code owner

@thrix thrix Awaiting requested review from thrix thrix is a code owner

@janhavlin janhavlin Awaiting requested review from janhavlin janhavlin is a code owner

@martinhoyer martinhoyer Awaiting requested review from martinhoyer martinhoyer is a code owner

Assignees

@psss psss

Labels
ci | full test Pull request is ready for the full test execution code | no functional change "No Functional Change" intended. Patch should not change tmt's behavior in any way. code | style Code style changes not affecting functionality priority | should medium priority, should be included in the next release status | ready for merge The only missing piece is to do the rebase the current 'main' and let the CI finish. step | prepare Stuff related to the prepare step
Projects
None yet
Milestone
1.42
Development

Successfully merging this pull request may close these issues.
5 participants
@happz @falconizmi @psss @henrywang @skycastlelily