chore: Switch from deprecated attributes for VPC Prefix create/update request to Site#423
chore: Switch from deprecated attributes for VPC Prefix create/update request to Site#423thossain-nv wants to merge 1 commit intomainfrom
Conversation
… Site request Signed-off-by: Tareque Hossain <thossain@nvidia.com>
thossain-nv
requested review from
hwadekar-nv,
kfelternv,
parmani-nv and
pbreton
Summary by CodeRabbit
WalkthroughThis pull request refactors VPC prefix request handling to adopt nested Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-04-24 01:09:12 UTC | Commit: fe90a12 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
api/pkg/api/handler/vpcprefix_test.go (1)
531-540: Assert the new workflow ID contract as well.These checks validate the payload shape, but the PR also changes
StartWorkflowOptions.IDtovpc-prefix-*. Without asserting the options argument, that behavioral change is still untested.Also applies to: 1521-1529
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/pkg/api/handler/vpcprefix_test.go` around lines 531 - 540, The test currently asserts the workflow payload but not the StartWorkflowOptions.ID change; locate the ExecuteWorkflow call in tsc.Calls (the last call where Method == "ExecuteWorkflow") and additionally extract the options argument (the StartWorkflowOptions argument passed to ExecuteWorkflow, e.g., at the index before the workflow/request arguments) and assert its ID equals the expected "vpc-prefix-<id>" value derived from resp.Id (or resp.Name if your contract uses Name); add the assertion alongside the existing checks for cwssaws.VpcPrefixCreationRequest so the test verifies the new workflow ID contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/pkg/api/handler/vpcprefix_test.go`:
- Line 1519: The assertion is comparing resp.Updated against the original
vpcprefix instead of the entity that was modified (vpcprefix2), so replace the
reference to vpcprefix with vpcprefix2 in the failing check: assert.NotEqual(t,
resp.Updated.String(), vpcprefix2.Updated.String()). Update the assertion in the
test that covers "success case when name is updated with the same value" to
compare resp.Updated to vpcprefix2.Updated (the updated entity) so the test
verifies the updated record’s timestamp actually changed.
In `@api/pkg/api/handler/vpcprefix.go`:
- Line 279: The workflow ID string was changed from the historically-stable
format to a new value, breaking Temporal deduplication; revert the
StartWorkflowOptions.ID construction to the original stable format (use the
exact prior prefix used for this workflow, e.g.
"vpcprefix-create-"+vpcPrefix.ID.String()) wherever the handler builds the ID
(refer to StartWorkflowOptions.ID and the code that concatenates
vpcPrefix.ID.String()), and ensure the same exact ID format is used consistently
in the other affected spot (the handler code that references the workflow ID at
the second location) so duplicate protection remains effective.
In `@api/pkg/api/model/vpcprefix.go`:
- Around line 98-101: The validation currently returns a hardcoded
"prefixLength" error when either vpur.IPBlockID or vpur.PrefixLength is set,
which misattributes failures; update the validation in the Validate method that
checks vpur.IPBlockID and vpur.PrefixLength to return the correct field key(s):
use "ipBlockId" when vpur.IPBlockID != nil, "prefixLength" when
vpur.PrefixLength != nil, and include both entries if both are non-nil (build a
validation.Errors map accordingly) instead of always returning "prefixLength".
---
Nitpick comments:
In `@api/pkg/api/handler/vpcprefix_test.go`:
- Around line 531-540: The test currently asserts the workflow payload but not
the StartWorkflowOptions.ID change; locate the ExecuteWorkflow call in tsc.Calls
(the last call where Method == "ExecuteWorkflow") and additionally extract the
options argument (the StartWorkflowOptions argument passed to ExecuteWorkflow,
e.g., at the index before the workflow/request arguments) and assert its ID
equals the expected "vpc-prefix-<id>" value derived from resp.Id (or resp.Name
if your contract uses Name); add the assertion alongside the existing checks for
cwssaws.VpcPrefixCreationRequest so the test verifies the new workflow ID
contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dbf8f08b-5319-4b44-b08c-a4db4e4e6738
📒 Files selected for processing (3)
api/pkg/api/handler/vpcprefix.goapi/pkg/api/handler/vpcprefix_test.goapi/pkg/api/model/vpcprefix.go
| assert.Equal(t, 1, len(resp.StatusHistory)) | ||
| assert.Equal(t, tc.expectedName, resp.Name) | ||
|
|
||
| assert.NotEqual(t, resp.Updated.String(), vpcprefix.Updated.String()) |
There was a problem hiding this comment.
Compare Updated against the entity under test.
Line 1519 always uses vpcprefix.Updated, but the "success case when name is updated with the same value" path updates vpcprefix2. That assertion can pass without proving the updated record’s timestamp moved.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/pkg/api/handler/vpcprefix_test.go` at line 1519, The assertion is
comparing resp.Updated against the original vpcprefix instead of the entity that
was modified (vpcprefix2), so replace the reference to vpcprefix with vpcprefix2
in the failing check: assert.NotEqual(t, resp.Updated.String(),
vpcprefix2.Updated.String()). Update the assertion in the test that covers
"success case when name is updated with the same value" to compare resp.Updated
to vpcprefix2.Updated (the updated entity) so the test verifies the updated
record’s timestamp actually changed.
|
|
||
| workflowOptions := temporalClient.StartWorkflowOptions{ | ||
| ID: "vpcprefix-create-" + vpcPrefix.ID.String(), | ||
| ID: "vpc-prefix-create-" + vpcPrefix.ID.String(), |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Temporal workflow duplicate ID behavior: if one deployment starts a workflow with ID "vpcprefix-create-<uuid>" and a later retry starts "vpc-prefix-create-<uuid>", will Temporal treat them as distinct workflows and bypass duplicate-workflow protection?
💡 Result:
No, Temporal will treat "vpcprefix-create-<uuid>" and "vpc-prefix-create-<uuid>" as distinct workflow IDs because they differ in case (P vs p) and hyphen placement. Workflow ID uniqueness is enforced with exact string matching, which is case-sensitive. Duplicate protection (via WorkflowIdReusePolicy and WorkflowIdConflictPolicy) only applies to identical Workflow IDs within the same Namespace. Temporal guarantees that only one Open Workflow Execution per exact Workflow ID can exist at a time, regardless of policy. Policies like RejectDuplicate or default Fail (for conflicts) prevent starting if an identical ID is Open or (for reuse) Closed with certain statuses. Since these IDs are different strings, protection is bypassed, allowing both to run concurrently.
Citations:
- 1: https://docs.temporal.io/workflow-execution/workflowid-runid
- 2: https://php.temporal.io/classes/Temporal-Common-IdReusePolicy.html
- 3: https://ruby.temporal.io/Temporalio/WorkflowIDReusePolicy.html
- 4: https://dotnet.temporal.io/api/Temporalio.Api.Enums.V1.WorkflowIdReusePolicy.html
- 5: https://www.javadoc.io/static/io.temporal/temporal-serviceclient/1.22.1/io/temporal/api/enums/v1/WorkflowIdReusePolicy.html
- 6: https://nodejs.temporal.io/api/enums/proto.temporal.api.enums.v1.WorkflowIdReusePolicy
Preserve workflow ID stability across deployments.
Changing StartWorkflowOptions.ID from vpcprefix-* to vpc-prefix-* breaks Temporal's duplicate workflow protection. Temporal uses exact string matching (case-sensitive) for workflow IDs; these distinct strings will bypass deduplication. During rolling deployments or retries across versions, an old pod can start a workflow with ID vpcprefix-create-<uuid> and a retry through a new pod starts vpc-prefix-create-<uuid>—Temporal treats these as separate workflows and executes the operation twice.
Maintain a stable workflow ID format across rollout boundaries, or implement explicit deduplication logic in the handler layer.
Affects lines 279 and 836.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/pkg/api/handler/vpcprefix.go` at line 279, The workflow ID string was
changed from the historically-stable format to a new value, breaking Temporal
deduplication; revert the StartWorkflowOptions.ID construction to the original
stable format (use the exact prior prefix used for this workflow, e.g.
"vpcprefix-create-"+vpcPrefix.ID.String()) wherever the handler builds the ID
(refer to StartWorkflowOptions.ID and the code that concatenates
vpcPrefix.ID.String()), and ensure the same exact ID format is used consistently
in the other affected spot (the handler code that references the workflow ID at
the second location) so duplicate protection remains effective.
| if vpur.IPBlockID != nil || vpur.PrefixLength != nil { | ||
| return validation.Errors{ | ||
| "prefix": errors.New("update has not been supported yet, only name allowed"), | ||
| "prefixLength": errors.New("prefix length modification is not supported at this time"), | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Return the rejected field name accurately.
When only ipBlockId is present, this now reports a prefixLength validation error. That misattributes the failure and will point clients at the wrong field.
Proposed fix
if vpur.IPBlockID != nil || vpur.PrefixLength != nil {
- return validation.Errors{
- "prefixLength": errors.New("prefix length modification is not supported at this time"),
- }
+ errs := validation.Errors{}
+ if vpur.IPBlockID != nil {
+ errs["ipBlockId"] = errors.New("IP block modification is not supported at this time")
+ }
+ if vpur.PrefixLength != nil {
+ errs["prefixLength"] = errors.New("prefix length modification is not supported at this time")
+ }
+ return errs
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if vpur.IPBlockID != nil || vpur.PrefixLength != nil { | |
| return validation.Errors{ | |
| "prefix": errors.New("update has not been supported yet, only name allowed"), | |
| "prefixLength": errors.New("prefix length modification is not supported at this time"), | |
| } | |
| if vpur.IPBlockID != nil || vpur.PrefixLength != nil { | |
| errs := validation.Errors{} | |
| if vpur.IPBlockID != nil { | |
| errs["ipBlockId"] = errors.New("IP block modification is not supported at this time") | |
| } | |
| if vpur.PrefixLength != nil { | |
| errs["prefixLength"] = errors.New("prefix length modification is not supported at this time") | |
| } | |
| return errs | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/pkg/api/model/vpcprefix.go` around lines 98 - 101, The validation
currently returns a hardcoded "prefixLength" error when either vpur.IPBlockID or
vpur.PrefixLength is set, which misattributes failures; update the validation in
the Validate method that checks vpur.IPBlockID and vpur.PrefixLength to return
the correct field key(s): use "ipBlockId" when vpur.IPBlockID != nil,
"prefixLength" when vpur.PrefixLength != nil, and include both entries if both
are non-nil (build a validation.Errors map accordingly) instead of always
returning "prefixLength".
1 participant
Description
VPC Prefix create and update API handlers are currently using attributes that have been deprecated in Core proto. This PR updates the requests to use newer attributes.
Type of Change
Services Affected
Related Issues (Optional)
None
Breaking Changes
Testing
Additional Notes
None