middleware icon indicating copy to clipboard operation
middleware copied to clipboard

Published 20 hours ago • verified publisher icon truenas

NAS-128590 / 24.10 / Fix Range validator args

Open sonicaj opened this issue 9 months ago • 5 comments

This commit fixes an issue where when we change max to max_ in middleware api for validators, this change was not addressed.

sonicaj avatar May 13 '24 18:05 sonicaj

Jira URL: https://ixsystems.atlassian.net/browse/NAS-128590

bugclerk avatar May 13 '24 18:05 bugclerk

@sonicaj why is this a draft?

yocalebo avatar May 13 '24 18:05 yocalebo

@yocalebo that is the max/min app devs specify in questions.yaml - we internally changed our API to use max_ instead. So it seems very weird to use max_ in yaml file to outline maximum length, we internally changed it because it collided with python builtins. Also it would be very messy to have app devs switch to max_ if we wanted to (i don't think we should), as older apps which app devs have not updated will no longer work and would be a decent change depending on the size of catalog. I think we should have tests in place to catch this sooner, i'll write some and push them if that sounds good ?

sonicaj avatar May 13 '24 19:05 sonicaj

backport to 24.04.1 as well please

ksimm1 avatar May 13 '24 20:05 ksimm1

@yocalebo that is the max/min app devs specify in questions.yaml - we internally changed our API to use max_ instead. So it seems very weird to use max_ in yaml file to outline maximum length, we internally changed it because it collided with python builtins. Also it would be very messy to have app devs switch to max_ if we wanted to (i don't think we should), as older apps which app devs have not updated will no longer work and would be a decent change depending on the size of catalog. I think we should have tests in place to catch this sooner, i'll write some and push them if that sounds good ?

Yep agree, tests would be a good start!

yocalebo avatar May 13 '24 20:05 yocalebo

Are you sure this fixes all the failures? On lines 108, 109 I still see references to min and max

yocalebo avatar May 14 '24 23:05 yocalebo

Are you sure this fixes all the failures? On lines 108, 109 I still see references to min and max

@yocalebo yes I am and i have added a unit test as well to outline that. Those min/max references are to the values in questions.yaml and not our schema so we are safe on that end

sonicaj avatar May 15 '24 01:05 sonicaj

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.

bugclerk avatar May 15 '24 11:05 bugclerk