Repository navigation
Conversation
dce9828 to
704c430
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
704c430 to
1a35604
Compare
1a35604 to
0a3bb91
Compare
0a3bb91 to
cd70d5e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd70d5e6bb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
cd70d5e to
11f685d
Compare
| --deployment-name YourDeploymentName \ | ||
| --build-id YourBuildID \ | ||
| --scaling-group YourScalingGroup \ | ||
| --region-id aws-us-west-2 |
There was a problem hiding this comment.
"To update only the region of a scaling group, supply --region-id without the other flags."
What if --region-id is supplied, with a --scaling-group NotFoundGroup ? Will it a group with only region-id? Or will it fail? I think this scenario can apply to PR #1235 as well, for flags other than --region-id, that use invalid scaling-group.
There was a problem hiding this comment.
Yes it will fail with a message indicating that the scaling group doesn't exist.
There was a problem hiding this comment.
correction - update doesn’t require the scaling group to exist. With a --scaling-group that doesn't exist, it adds a new scaling group, and the server validates it
- With the provider flags, it creates the group.
- With only --region-id, there’s no provider, so the server rejects it:
Error: error updating worker deployment version compute config: entry not-found-group: invalid compute provider type ''
A new group in a region that already has one is also rejected: “only one scaling group in region … can have no task types defined”.
0b5d68f to
4b5580b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b5580bf82
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if o.RegionId == "" { | ||
| return nil, fmt.Errorf("--region-id cannot be empty") | ||
| } | ||
| sg.RegionIds = []string{o.RegionId} |
There was a problem hiding this comment.
Preserve regions in version describe output
When a scaling group is created with --region-id, worker deployment version describe -o json silently drops that routing information: formatComputeConfigProto in commands.worker.deployment.go:506-539 copies only the provider and scaler into formattedComputeConfigScalingGroup. Consequently, the general version-description command reports regional and fallback groups with otherwise identical JSON, so clients cannot reconstruct the configuration; add the region IDs to that formatter as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
version describe keeps the same output as describe-version on main, that formatter shows each scaling group’s provider and scaler only, we can keep it the same and let users use compute-config describe for a detailed lookup response
There was a problem hiding this comment.
@ks-temporal I’ve added regionId to each scaling group in the JSON output. I’d left it out at first because describe-version shares this formatter, but the field is omitted when a scaling group has no region. Regions can only be set through the new commands, so existing output doesn’t change.
I kept the text ComputeConfigSummary as it is on main. Adding regions there would read like aws-lambda,aws-lambda in aws-us-east-1,aws-lambda in aws-us-west-2, which doesn't seem useful to me. We can improve later on if needed.
4b5580b to
fc81db4
Compare
| github.com/temporalio/ui-server/v2 v2.54.1 | ||
| go.temporal.io/api v1.63.5 | ||
| go.temporal.io/sdk v1.47.0 | ||
| go.temporal.io/api v1.64.0 |
There was a problem hiding this comment.
If there is an API version bump, then this PR will need to wait until the next OSS server release to be part of the generic CLI release. Alternatively, it could go in cloud specific pre-releases of CLI but not the general CLI release.
If the --region-id feature is independent of the API version/change AND if you want these CLI PRs/changes to go in the end-of-month CLI release (without a new OSS server release), then I recommend using older api version (v1.63.5) that is part of the currently released OSS server.
On the other hand if the --region-id depends on the new OSS server release, then this is fine - just will wait until OSS server release.
There was a problem hiding this comment.
Does this mean the breaking changes checkbox should be unchecked in the PR description?
There was a problem hiding this comment.
I don't think so. This is not a breaking change from what I can tell. The CLI build process will ensure that if this PR is included in the CLI release, then it will have an embedded server which uses an API version of 1.64.0 or higher; otherwise this PR will not be included in that CLI release.
From what I know, a breaking change is like removing an already defined option or command, or changing the semantics of the existing option or command. Adding a new option or bumping the API version to use newer API/SDK is not a breaking change.
This PR will likely be included in either a cloud-tagged pre-release CLI, which usually builds from new/recent tag from server "main" branch (which can be done anytime on demand), or the public CLI release after the next public OSS server release (which I believe is planned for mid Nov but could be delayed).
gcristea-temporal
left a comment
There was a problem hiding this comment.
Looks good, thanks for clarifying the intent here.
|
Is "Error: error updating worker deployment version compute config: entry not-found-group: only one scaling group in region aws-us-east-1 can have no task types defined" the expected output for the not found scaling group update? It's the last error cases example |
Related issues
temporalio/api#875 (
region_ids), released in api-gov1.64.0. Stacked on the compute-config command tree PR.What changed?
aws-lambda createandaws-lambda updatetake--region-id, a single region. A scaling group with a region is only used while the Namespace is active in that region. Without--region-id, a new scaling group can be used in any region.aws-lambda update --region-idon its own changes only the region. The region passed replaces the existing one, and other updates keep it.compute-config describeshows each scaling group's regionversion describeanddescribe-versionshow each scaling group'sregionIdin JSON, only when it has one, so output for scaling groups without a region is unchanged. The one-lineComputeConfigSummarystays as on main.go.temporal.io/apitov1.64.0, the first release withregion_ids, andgo.temporal.io/sdktov1.49.0, which renamesStartActivityOptions.DetailstoStaticDetails.Checklist
commands.yamlSharedServerSuite)Manual tests
I ran these against a Temporal Cloud test cell with a Namespace replicated across
aws-us-west-2andaws-us-east-1, and an AWS Lambda function in each region. All of them behaved as expected.Happy path
Add scaling groups for both regions, and a fallback without a region:
version describekeeps the same one-line summary as on main, and its JSON showsregionIdon the regional scaling groups only:--region-idon its own changes only the region:Other updates keep the region:
Error cases
The last one shows that
updateon a scaling group that doesn't exist is treated as adding a new scaling group, so the server's one-group-per-region check applies to it.Composition: remove the
aws-us-west-2scaling group, so thedefaultfallback serves that region