worker deployment update-version-compute-config#1001
Conversation
jaypipes
force-pushed
the
jaypipes/replace-version-compute-config
branch
from
afab676 to
f6fdb9b
Compare
jaypipes
changed the title
jaypipes
force-pushed
the
jaypipes/replace-version-compute-config
branch
2 times, most recently
from
f6601f4 to
bdd4c48
Compare
|
@chaptersix I don't believe the test failure has anything to do with this particular PR. |
|
A couple things worth reconsidering here: Command naming -- The CLI uses Implicit delete behavior -- Currently if |
@chaptersix all great points. I will rework the patch! |
Adds implementation of the `UpdateWorkerDeploymentVersionComputeConfig` gRPC API call with a `temporal worker deployment replace-version-compute-config` CLI command. I used `replace` instead of `update` to be more explicit about the replacement semantics used by the command. Signed-off-by: Jay Pipes <jay.pipes@temporal.io>
Changes the `temporal worker deployment replace-version-compute-config` CLI command to `temporal worker deployment update-version-compute-config` to match other CLI commands. Adds an explicit `--remove` CLI flag to remove the compute config on the version. Signed-off-by: Jay Pipes <jay.pipes@temporal.io>
jaypipes
force-pushed
the
jaypipes/replace-version-compute-config
branch
from
bdd4c48 to
6bedccf
Compare
|
@chaptersix sorry for the delay and thank you for your patience. I've updated the code per your review recommendations. |
jaypipes
changed the title
| RequestId: requestID, | ||
| } | ||
|
|
||
| if c.Remove { |
There was a problem hiding this comment.
Though AwsLambdaFunctionArn, AwsLambdaAssumeRoleArn, AwsLambdaAssumeRoleExternalId are harmlessly ignored when --remove is passed in, there's precedence in this repo (i.e. see TemporalOperatorNexusEndpointCreateCommand) to guard against non-supported permutations. Maybe a guard like this in the top of the method:
if c.Remove && (c.AwsLambdaFunctionArn != "" || c.AwsLambdaAssumeRoleArn != "" || c.AwsLambdaAssumeRoleExternalId != "") {
return fmt.Errorf("--remove cannot be combined with --aws-lambda-function-arn, --aws-lambda-assume-role-arn, or --aws-lambda-assume-role-external-id")
}
|
|
||
| // awsLambdaProviderDetailsPayload returns the encoded Payload representing AWS | ||
| // Lambda compute provider details. | ||
| func (c *TemporalWorkerDeploymentUpdateVersionComputeConfigCommand) awsLambdaProviderDetailsPayload() (*commonpb.Payload, error) { |
There was a problem hiding this comment.
I think this looks functionally identical to TemporalWorkerDeploymentCreateVersionCommand.awsLambdaProviderDetailsPayload? Maybe refactor into a helper func
| if c.AwsLambdaAssumeRoleExternalId != "" { | ||
| providerDetails["role_external_id"] = c.AwsLambdaAssumeRoleExternalId | ||
| } | ||
| err := validateAWSLambdaProviderDetails(providerDetails) |
There was a problem hiding this comment.
Seems this will reject if all three AWS Lambda flags are not supplied together. Maybe worth it to specify in help all 3 must be supplied together
3 participants
Adds implementation of the
UpdateWorkerDeploymentVersionComputeConfiggRPC API call with atemporal worker deployment update-version-compute-configCLI command. There is a--removeCLI flag that allows the user to explicitly delete the compute config from a Worker Deployment Version.