Be more generous validating update project #145
Conversation
honnix
requested review from
anandswaminathan,
EngHabu,
katrogan,
kumare3,
matthewphsmith and
wild-endeavor
as code owners
| if err := ValidateEmptyStringField(project.Name, projectName); err != nil { | ||
| return err | ||
| } | ||
| return ValidateProject(project) |
There was a problem hiding this comment.
do you want to move more of the validation (e.g. description, labels, etc) out of ValidateProject and into this method?
There was a problem hiding this comment.
oh perhaps labels is unnecessary, but description maybe
There was a problem hiding this comment.
We are not validating emptiness of description in VadlidateProject. Do you mean we want to enforce non-empty for description as well?
There was a problem hiding this comment.
ah you're right again, sorry skimmed over that
Codecov Report
@@ Coverage Diff @@
## master #145 +/- ##
=======================================
Coverage 61.79% 61.79%
=======================================
Files 105 105
Lines 7533 7534 +1
=======================================
+ Hits 4655 4656 +1
Misses 2286 2286
Partials 592 592
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
* Be more generous validating update project
TL;DR
#132 added support for validating projects for both create and update. However when updating a project, it is reasonable to have some fields unset (empty string coming from gRPC). In case of archiving/activating a project, only project ID and state are required.
Type
Are all requirements met?
Complete description
Executing for example
flyte-cli archive-project -i -h <host:port> -p some-projectcan easily reproduce.This PR skips validating empty string for
project.Namewhen it is an update operation. Actually even for a create operation, it should be OK to leaveproject.Nameunset, but to keep backward compatibility, this is still enforced.Tracking Issue
flyteorg/flyte#336
Follow-up issue
NA