feat: Add support for creating a database semaphore config using CLI #14784
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]>
…ixes argoproj#14783 Signed-off-by: Darko Janjic <[email protected]>
…ixes argoproj#14783 Signed-off-by: Darko Janjic <[email protected]>
…ixes argoproj#14783 Signed-off-by: Darko Janjic <[email protected]>
…ixes argoproj#14783 Signed-off-by: Darko Janjic <[email protected]>
…ixes argoproj#14783 Signed-off-by: Darko Janjic <[email protected]>
…ixes argoproj#14783 Signed-off-by: Darko Janjic <[email protected]>
djanjic
force-pushed
the
db-sync-cli
branch
from
698ec1e to
be436b5
Compare
cmd/argo/commands/sync/db/create.go
Outdated
| Use: "create", | ||
| Short: "create a db sync limit", | ||
| Args: cobra.ExactArgs(1), | ||
| Example: `argo sync db create my-key --size-limit 10`, |
There was a problem hiding this comment.
While I understand why there is no --key flag here, I kind of don't like the inconsistency.
There was a problem hiding this comment.
Makes me wonder if the cli should look more like one of these:
argo sync create my-key --type db
argo sync create my-key --configmap cmapname
argo sync create my-key --type configmap --name cmapname
There was a problem hiding this comment.
I agree with this, consistency between the two commands would be preferable.
There was a problem hiding this comment.
my preferred solution would also be:
argo sync create --type [configmap|db] --limit [--configmap ]
There was a problem hiding this comment.
This is now updated. Both cm and db part.
cmd/argo/commands/sync/db/create.go
Outdated
| Use: "create", | ||
| Short: "create a db sync limit", | ||
| Args: cobra.ExactArgs(1), | ||
| Example: `argo sync db create my-key --size-limit 10`, |
There was a problem hiding this comment.
I agree with this, consistency between the two commands would be preferable.
docs/cli/argo_sync_db_create.md
Outdated
|
|
||
| ``` | ||
| -h, --help help for create | ||
| --size-limit int32 Size limit of the sync limit |
There was a problem hiding this comment.
I think this could be simplified to either --size or --limit here and in the other places
|
|
||
| func (s *dbSyncProvider) createSyncLimit(ctx context.Context, req *syncpkg.CreateSyncLimitRequest) (*syncpkg.SyncLimitResponse, error) { | ||
| allowed, err := auth.CanI(ctx, "create", workflow.WorkflowPlural, req.Namespace, "") | ||
| if err != nil { |
There was a problem hiding this comment.
just wondering, why do you think it is necessary to check permissions for DB sync limits but not for ConfigMap sync?
There was a problem hiding this comment.
This was my suggestion. ConfigMap access is controlled by Kubernetes standard RBAC. There is no such equivalent for the database access, so the idea was to make it use an in cluster object to see if the request was "reasonable".
@djanjic - this behaviour should be documented, and this whole class of db access should be controllable (e.g. the admin can disable it if they don't want it).
server/sync/sync_cm.go
Outdated
| if err == nil { | ||
| _, has := cm.Data[req.Key] | ||
| if has { | ||
| return nil, fmt.Errorf("sync limit cannot be created as it already exists") |
There was a problem hiding this comment.
this should return a status.Error(codes.AlreadyExists) error
…ixes argoproj#14783 Signed-off-by: Darko Janjic <[email protected]>
…ixes argoproj#14783 Signed-off-by: Darko Janjic <[email protected]>
…ixes argoproj#14783 Signed-off-by: Darko Janjic <[email protected]>
…ixes argoproj#14783 Signed-off-by: Darko Janjic <[email protected]>
…ixes argoproj#14783 Signed-off-by: Darko Janjic <[email protected]>
…ixes argoproj#14783 Signed-off-by: Darko Janjic <[email protected]>
…ixes argoproj#14783 Signed-off-by: Darko Janjic <[email protected]>
…ixes argoproj#14783 Signed-off-by: Darko Janjic <[email protected]>
…ixes argoproj#14783 Signed-off-by: Darko Janjic <[email protected]>
…rgoproj#14784) Signed-off-by: Darko Janjic <[email protected]>
…rgoproj#14784) Signed-off-by: Darko Janjic <[email protected]>
4 participants
Fixes #14783
Motivation
This adds support for creating a database semaphore configuration using the CLI.
Modifications
Verification
Documentation