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

NAS-132813 / 25.04 / Convert pool.snapshottask to new API #15164

Merged
merged 8 commits into from
Dec 10, 2024

Conversation

creatorcary
Copy link
Contributor

@creatorcary creatorcary commented Dec 9, 2024

When using AfterValidator, the passed callable's return value is assigned to the field. For this reason, our validators should return the value that is passed to them or some modified version of the value. This change should not cause any issues since we have not been using the validators' return values until now.

Tests: http://jenkins.eng.ixsystems.net:8080/job/tests/job/api_tests/2157/

@bugclerk bugclerk changed the title Convert pool.snapshottask to new API NAS-132813 / 25.04 / Convert pool.snapshottask to new API Dec 9, 2024
@bugclerk
Copy link
Contributor

bugclerk commented Dec 9, 2024

@creatorcary creatorcary marked this pull request as ready for review December 9, 2024 21:12
@creatorcary creatorcary requested a review from a team December 9, 2024 21:12
Copy link
Contributor

@themylogin themylogin left a comment

Choose a reason for hiding this comment

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

Also I don't understand why do you want to use that validator module (it'll be eventually removed) and change it so massively. Let's just re-implement that field type like we did with LocalUsername, LocalGID. Also, let's put it into base types since other classes will also use it.

@creatorcary
Copy link
Contributor Author

I was under the impression that we wanted to bring our old validators into the new API so I'll correct this.

Copy link
Contributor

@themylogin themylogin 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!

@creatorcary creatorcary merged commit 8af0dc7 into master Dec 10, 2024
2 checks passed
@creatorcary creatorcary deleted the NAS-132813 branch December 10, 2024 17:01
@bugclerk
Copy link
Contributor

This PR has been merged and conversations have been locked.
If you would like to discuss more about this issue please use our forums or raise a Jira ticket.

@truenas truenas locked as resolved and limited conversation to collaborators Dec 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants