Skip to content

Add Pydantic validation for reboot configuration #4356

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
Merged
happz merged 10 commits into from
Dec 1, 2025
Merged

Add Pydantic validation for reboot configuration #4356

happz merged 10 commits into from
Dec 1, 2025

Conversation

@AthreyVinay
Copy link
Contributor

@AthreyVinay AthreyVinay commented Nov 22, 2025 •
edited
Loading

Resolves #4349

Changes made:

  • Define RebootData Pydantic model to validate config structure
  • Simplify downstream logic since all fields are now guaranteed valid

A few TODOs to be reviewed.

@AthreyVinay AthreyVinay added this to the 1.63 milestone Nov 22, 2025
@AthreyVinay AthreyVinay added code | style Code style changes not affecting functionality command | reboot Support for rebooting guests during `tmt run` and the `tmt-reboot` command area | maintenance Changes important for efficiency and the long-term health of the project labels Nov 22, 2025
@github-project-automation github-project-automation bot moved this to backlog in planning Nov 22, 2025
@AthreyVinay AthreyVinay moved this from backlog to review in planning Nov 22, 2025
@AthreyVinay AthreyVinay added the review | trivial Very easy for review, even for beginners, so don't be afraid to have a look! :-) label Nov 22, 2025
@happz happz added the ci | full test Pull request is ready for the full test execution label Nov 22, 2025
happz
happz reviewed Nov 22, 2025
View reviewed changes
happz
happz reviewed Nov 22, 2025
View reviewed changes
LecrisUT
LecrisUT reviewed Nov 24, 2025
View reviewed changes
Copy link
Contributor

@LecrisUT LecrisUT 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, just needs to be pydantic v1 compatible.

@vaibhavdaren
Copy link
Contributor

vaibhavdaren commented Nov 25, 2025

The comments on this PR are nice to read. They give a clear understanding of base models. If our tests are failing probably they will fail for other users too. I think we should add a release note for this as well.

@AthreyVinay AthreyVinay force-pushed the avinay-validate-reboot-data branch from a927ad2 to 24f8aa3 Compare November 26, 2025 09:38
graphite-app[bot]
graphite-app bot reviewed Nov 26, 2025
View reviewed changes
@AthreyVinay
Copy link
Contributor Author

AthreyVinay commented Nov 26, 2025

I think we should add a release note for this as well.

I think its a valid comment - since the contract is now changed i.e. empty strings which were allowed will NOT be anymore. Anybody with more thoughts / opinions?

@LecrisUT
Copy link
Contributor

LecrisUT commented Nov 26, 2025

/packit build

@happz
Copy link
Contributor

happz commented Nov 26, 2025

I think we should add a release note for this as well.

I think its a valid comment - since the contract is now changed i.e. empty strings which were allowed will NOT be anymore. Anybody with more thoughts / opinions?

Yes, a release note can't hurt.

graphite-app[bot]
graphite-app bot reviewed Nov 26, 2025
View reviewed changes
happz
happz reviewed Nov 26, 2025
View reviewed changes
therazix
therazix reviewed Nov 27, 2025
View reviewed changes
@AthreyVinay AthreyVinay force-pushed the avinay-validate-reboot-data branch 3 times, most recently from 506c501 to 181226c Compare November 27, 2025 15:04
@AthreyVinay AthreyVinay moved this from review to merge in planning Nov 27, 2025
@happz happz force-pushed the avinay-validate-reboot-data branch from 181226c to 40e5d39 Compare November 28, 2025 15:31
@happz
Copy link
Contributor

happz commented Nov 30, 2025 •
edited
Loading

/packit build

happz
happz reviewed Nov 30, 2025
View reviewed changes
@happz happz moved this from merge to implement in planning Nov 30, 2025
@AthreyVinay AthreyVinay moved this from implement to review in planning Nov 30, 2025
@AthreyVinay AthreyVinay removed the review | trivial Very easy for review, even for beginners, so don't be afraid to have a look! :-) label Nov 30, 2025
@happz
Copy link
Contributor

happz commented Dec 1, 2025

Unrelated failures, merging.

@happz happz merged commit ca407e7 into main Dec 1, 2025
28 of 29 checks passed
@happz happz deleted the avinay-validate-reboot-data branch December 1, 2025 06:34
@github-project-automation github-project-automation bot moved this from review to done in planning Dec 1, 2025
@AthreyVinay AthreyVinay mentioned this pull request Dec 13, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@happz happz happz approved these changes

@LecrisUT LecrisUT LecrisUT approved these changes

@graphite-app graphite-app[bot] graphite-app[bot] left review comments

@vaibhavdaren vaibhavdaren vaibhavdaren approved these changes

@therazix therazix therazix approved these changes

Assignees

No one assigned

Labels

area | maintenance Changes important for efficiency and the long-term health of the project ci | full test Pull request is ready for the full test execution code | style Code style changes not affecting functionality command | reboot Support for rebooting guests during `tmt run` and the `tmt-reboot` command

Projects

planning
Status: done

Milestone

1.63

Development

Successfully merging this pull request may close these issues.

Validate reboot request file has the right format

6 participants

@AthreyVinay @vaibhavdaren @LecrisUT @happz @therazix