Checks to ensure Cluster name unique and required #13798
Conversation
prachidamle
changed the title
|
@alena1108 review pls. |
prachidamle
force-pushed
the
cluster_name_unique
branch
from
9ec8345
to
278d871
Compare
prachidamle
changed the title
|
|
||
| name, ok := data["name"].(string) | ||
| if !ok { | ||
| return nil, errors.New("Cluster name is required") |
There was a problem hiding this comment.
@prachidamle do we need this extra check? Given that required attribute is already specified as norman annotation
There was a problem hiding this comment.
@alena1108 yeah we need this check since the API request data is sent in a map and not as the Cluster object, so need to check the field to avoid panics while casting to string
| return nil, errors.New("Cluster name is required") | ||
| } | ||
|
|
||
| r.mu.Lock() |
There was a problem hiding this comment.
this code should be invoked on the update as well
There was a problem hiding this comment.
Added the update method
prachidamle
force-pushed
the
cluster_name_unique
branch
from
278d871
to
869e6b3
Compare
| r.mu.Lock() | ||
| defer r.mu.Unlock() | ||
|
|
||
| var clusters []managementv3.Cluster |
There was a problem hiding this comment.
we should perhaps extract the code into common method. From line 39 to line 51
|
|
||
| func (r *Store) Create(apiContext *types.APIContext, schema *types.Schema, data map[string]interface{}) (map[string]interface{}, error) { | ||
|
|
||
| name, ok := data["name"].(string) |
There was a problem hiding this comment.
we should extract lines 31-34 into common method
prachidamle
force-pushed
the
cluster_name_unique
branch
from
869e6b3
to
ca4c885
Compare
| clusterName = "" | ||
| } | ||
|
|
||
| updatedName, ok := data["name"].(string) |
There was a problem hiding this comment.
do we always expect the name in the update request? does UI send it all the time? if so, may be it's something we can check at the very top of the method, to save on querying the existing cluster if the name is missing. Also it can be extracted to its own method, so the call can replace line 31-34 in create
There was a problem hiding this comment.
Yeah name is always sent in the data[] for the UPdate call, infact all properties are present in the data[]. So to know if name is getting updated, we would have to compare with the existing name.
There was a problem hiding this comment.
Will swap the order of checking cluster name and pulling existing object. But just checking if clusterName is missing need not be a common method.
| clusterName = "" | ||
| } | ||
|
|
||
| updatedName, ok := data["name"].(string) |
There was a problem hiding this comment.
we should use convert library, otherwise there are chances of nil pointer. I believe it's convert.toString()
There was a problem hiding this comment.
Will check the library!
prachidamle
force-pushed
the
cluster_name_unique
branch
from
ca4c885
to
978a8e2
Compare
|
LGTM. @prachidamle could you remove this file from the vendor commit; usually binaries should not be included: |
prachidamle
force-pushed
the
cluster_name_unique
branch
from
978a8e2
to
e7d517d
Compare
#13075