feat: Add support for creating a configmap semaphore config using CLI. Fixes #14671 #14724
Conversation
…Fixes argoproj#14671 Signed-off-by: Darko Janjic <[email protected]>
…Fixes argoproj#14671 Signed-off-by: Darko Janjic <[email protected]>
…Fixes argoproj#14671 Signed-off-by: Darko Janjic <[email protected]>
…Fixes argoproj#14671 Signed-off-by: Darko Janjic <[email protected]>
…Fixes argoproj#14671 Signed-off-by: Darko Janjic <[email protected]>
…Fixes argoproj#14671 Signed-off-by: Darko Janjic <[email protected]>
…Fixes argoproj#14671 Signed-off-by: Darko Janjic <[email protected]>
djanjic
requested review from
Joibel,
sarabala1979 and
terrytangyuan
as code owners
eduardodbr
left a comment
eduardodbr
left a comment
There was a problem hiding this comment.
small questions but otherwise looks good
server/sync/sync_server.go
Outdated
| } | ||
|
|
||
| func (s *syncServer) DeleteSyncLimit(ctx context.Context, req *syncpkg.DeleteSyncLimitRequest) (*syncpkg.DeleteSyncLimitResponse, error) { | ||
| fmt.Printf("Deleting sync limit for ConfigMap %s in namespace %s with key %s\n", req.Name, req.Namespace, req.Key) |
There was a problem hiding this comment.
You can remove this print or replace it proper logging
| Use: "get", | ||
| Short: "Get a configmap sync limit", | ||
| Args: cobra.ExactArgs(1), | ||
| Example: `argo sync get configmap my-cm --key my-key`, |
There was a problem hiding this comment.
| Example: `argo sync get configmap my-cm --key my-key`, | |
| Example: `argo sync configmap get my-cm --key my-key`, |
the command and the method are switched
server/sync/sync_server.go
Outdated
| Key: req.Key, | ||
| SizeLimit: req.SizeLimit, | ||
| }, false) | ||
| } |
There was a problem hiding this comment.
A Create doing an Update violates some API principles, is this common in the current codebase?
There was a problem hiding this comment.
Yeah I agree with this, we should fail on any error really.
There was a problem hiding this comment.
This change should create a cm with a defined key/value for the semaphore. If cm already exists, we are just trying to add another field to that configmap, which could have multiple semaphores for different workflows. At least, that's the idea.
There was a problem hiding this comment.
I mostly agree with @djanjic, it's not completely pure:
I think this should:
- If the CM does not exist, create it (we probably all agree on this)
- If the CM does exist, but the key does not, create the key within in (by performing a CM update call).
- If the CM does exist and the key also does, fail.
| return nil, sutils.ToStatusError(err, codes.Internal) | ||
| } | ||
|
|
||
| delete(cm.Data, req.Key) |
There was a problem hiding this comment.
if the deleted key is the only key in the Configmap, should the CM also be deleted? otherwise the CM will be created with no data
There was a problem hiding this comment.
I think its fine to leave it around
docs/cli/argo_sync_configmap_get.md
Outdated
| ### Examples | ||
|
|
||
| ``` | ||
| argo sync get configmap my-cm --key my-key |
There was a problem hiding this comment.
| argo sync get configmap my-cm --key my-key | |
| argo sync configmap get my-cm --key my-key |
…Fixes argoproj#14671 Signed-off-by: Darko Janjic <[email protected]>
…Fixes argoproj#14671 Signed-off-by: Darko Janjic <[email protected]>
isubasinghe
left a comment
isubasinghe
left a comment
There was a problem hiding this comment.
We shouldn't attempt an update, apart from this issue, it looks good.
server/sync/sync_server.go
Outdated
| Key: req.Key, | ||
| SizeLimit: req.SizeLimit, | ||
| }, false) | ||
| } |
There was a problem hiding this comment.
Yeah I agree with this, we should fail on any error really.
| return nil, sutils.ToStatusError(err, codes.Internal) | ||
| } | ||
|
|
||
| delete(cm.Data, req.Key) |
There was a problem hiding this comment.
I think its fine to leave it around
Signed-off-by: isubasinghe <[email protected]>
Signed-off-by: isubasinghe <[email protected]>
…Fixes argoproj#14671 (argoproj#14724) Signed-off-by: Darko Janjic <[email protected]> Signed-off-by: isubasinghe <[email protected]> Co-authored-by: isubasinghe <[email protected]>
…Fixes argoproj#14671 (argoproj#14724) Signed-off-by: Darko Janjic <[email protected]> Signed-off-by: isubasinghe <[email protected]> Co-authored-by: isubasinghe <[email protected]>
…Fixes argoproj#14671 (argoproj#14724) Signed-off-by: Darko Janjic <[email protected]> Signed-off-by: isubasinghe <[email protected]> Co-authored-by: isubasinghe <[email protected]>
4 participants
Fixes #14671
Motivation
This adds support for creating configmap sync configuration using CLI.
Modifications
Verification
Documentation