feat: support grpc message size as env #17728
Conversation
Signed-off-by: pashakostohrys <[email protected]>
|
I like the idea of a single env var. We can always add component-specific vars later if we want. Could we make these configurable via argocd-cmd-params-cm? Doesn't have to set command flags like the other config values, it could just set the env var. |
common/common.go
Outdated
| @@ -273,6 +273,8 @@ const ( | |||
| // EnvServerSideDiff defines the env var used to enable ServerSide Diff feature. | |||
| // If defined, value must be "true" or "false". | |||
| EnvServerSideDiff = "ARGOCD_APPLICATION_CONTROLLER_SERVER_SIDE_DIFF" | |||
| // EnvArgoCDgRPCMaxSizeMB is the environment variable to look for a max gRPC message size | |||
| EnvArgoCDgRPCMaxSizeMB = "ARGOCD_GRPC_MAX_SIZE_MB" | |||
There was a problem hiding this comment.
Just a nit, and I know that it was like this before, but I think we do not need to include "ArgoCD" in the environment variable name. Also, I find the casing for gRPC odd in the context of a variable name. Can we make it either EnvGrpcMaxSize or EnvGRPCMaxSize (I prefer the former)?
There was a problem hiding this comment.
The problem, that this variable already exist, and it will be breaking change
There was a problem hiding this comment.
I'm not talking about the name of the environment variable itself (ARGOCD_GRPC_MAX_SIZE_MB), but the variable name used in the code (EnvArgoCDgRPCMaxSizeMB). I don't see renaming the Go variable to be a breaking change.
There was a problem hiding this comment.
sorry, my initial comment might have been misleading, because I wrote "environment variable name"
There was a problem hiding this comment.
So, to clarify:
EnvArgoCDgRPCMaxSizeMB should rather be EnvGrpcMaxSizeMB or EnvGRPCMaxSizeMB, with a preference to the former.
Signed-off-by: pashakostohrys <[email protected]>
…nto feat/grpc-message-size-as-env
Done :) |
The only question, if we want to create separate envs for each client, from one side it will increase complexity , but from another side it will reduce risk for existing users. Open to discuss