[SDK] improve PVC creation name error #2496
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…h correct name example Signed-off-by: mahdikhashan <[email protected]>
Signed-off-by: mahdikhashan <[email protected]>
mahdikhashan
force-pushed
the
improve-pvc-error-message
branch
from
7036b29 to
e005825
Compare
Signed-off-by: mahdikhashan <[email protected]>
| # RFC 1123 regex for valid PVC names: lowercase alphanumeric, '-', or '.'. | ||
| return bool( | ||
| re.match( | ||
| r"^[a-z0-9]([a-z0-9\-]*[a-z0-9])?(\.[a-z0-9]([a-z0-9\-]*[a-z0-9])?)*$", name |
There was a problem hiding this comment.
Would using the same regex format as shown in the error message improve readability and maintainability?
There was a problem hiding this comment.
would you please elaborate what you mean here?
There was a problem hiding this comment.
I mean using this regex format in the ValueError message: '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*', since it took me a while to compare if they stand for the same thing. Or is there specific reason you changed the format a little bit?
There was a problem hiding this comment.
ok, thanks for bringing this to my attention. shall i add any unit test for it? please let me know.
There was a problem hiding this comment.
also, there are two failing ci tests, i'm guessing they are flaky tests, would there be any problem that this change have caused?
There was a problem hiding this comment.
Since the unit tests for tune API is still under review, I think you can add your unit test after that one is merged.
There was a problem hiding this comment.
I think the CI test failures are caused by resource problems. I've rerun the tests once, but one of them still failed due to network connectivity issue. Maybe we can try running them again later.
There was a problem hiding this comment.
thanks for your time and help in this matter.
|
/rerun-all |
1 similar comment
|
/rerun-all |
What this PR does / why we need it:
This PR handles potential name errors for PVCs gracefully.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #2491
Checklist: