[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 |
3 similar comments
|
/rerun-all |
|
/rerun-all |
|
/rerun-all |
|
Thanks for the contribution! /lgtm |
is it fine if i add a few unit tests after the main unit test pr got merged - keeping this pr open till then? |
|
/assign @tenzen-y @andreyvelich |
The main unit test is already approved, but it seems the CI test is still in progress. I think we can merge this PR first, and it would be better to open a new PR to add unit tests for this. |
| @@ -557,6 +557,20 @@ class name in this argument. | |||
| # Create PVC for the Storage Initializer. | |||
| # TODO (helenxie-bit): PVC Creation should be part of Katib Controller. | |||
| try: | |||
| if not utils.is_valid_pvc_name(name): | |||
There was a problem hiding this comment.
I am wondering what is the goal to introduce additional validation on top of Kubernetes default validation ?
Are we trying to make this message more user friendly ?
cc @kubeflow/wg-training-leads
There was a problem hiding this comment.
Yes, I think that's the point.
There was a problem hiding this comment.
yes, basically my goal was to make the message user friendlier. i'm open to suggestions.
There was a problem hiding this comment.
Can we reconcile this error similar to 409 (CRD is already exist), so we won't introduce additional validation ?
katib/sdk/python/v1beta1/kubeflow/katib/api/katib_client.py
Lines 138 to 143 in 05dbea6
There was a problem hiding this comment.
yes, you are right - i'll change it.
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: