Skip to content

[bugfix] Fix aci_physical_domain resource deletion when the object is referenced by another object (DCNE-677)#1458

Open
sajagana wants to merge 2 commits intoCiscoDevNet:masterfrom
sajagana:677_aci_physical_domain_fix
Open

[bugfix] Fix aci_physical_domain resource deletion when the object is referenced by another object (DCNE-677)#1458
sajagana wants to merge 2 commits intoCiscoDevNet:masterfrom
sajagana:677_aci_physical_domain_fix

Conversation

@sajagana
Copy link
Copy Markdown
Collaborator

No description provided.

@sajagana sajagana force-pushed the 677_aci_physical_domain_fix branch from 001894d to b3384ee Compare March 25, 2026 13:38
@github-actions github-actions bot changed the title [bugfix] Fix aci_physical_domain resource deletion when the object is referenced by another object [bugfix] Fix aci_physical_domain resource deletion when the object is referenced by another object (DCNE-677) Mar 25, 2026
}

if restResponse != nil && cont.Data() != nil && restResponse.StatusCode != 200 {
if restResponse != nil && cont.Data() != nil && restResponse.StatusCode >= 400 {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep the != 200 here compared to go client to avoid changing something we are not aware of. This should not be the case but just in case. @samiib what is your take on this? In go client is was completely new code and here code is changed

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a little suspicious that we have already been treating any non-200 even other 2xx code as errors.
However, this is an existing check instead of a completely new conditional branch like before.
We can leave it as is and only add the errCode 107 check to fix the bug to be safe.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted the condition to original state.

}

if restResponse != nil && cont.Data() != nil && restResponse.StatusCode != 200 {
if restResponse != nil && cont.Data() != nil && restResponse.StatusCode >= 400 {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a little suspicious that we have already been treating any non-200 even other 2xx code as errors.
However, this is an existing check instead of a completely new conditional branch like before.
We can leave it as is and only add the errCode 107 check to fix the bug to be safe.

go 1.25.0

toolchain go1.24.5
toolchain go1.25.8
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the go version be modified as part of this PR?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved by the latest master.

@sajagana sajagana force-pushed the 677_aci_physical_domain_fix branch from b3384ee to dcbd2e9 Compare March 27, 2026 08:01
@sajagana sajagana requested review from akinross and samiib March 27, 2026 08:09
Copy link
Copy Markdown
Collaborator

@samiib samiib left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Terraform apply is successful however Error 400 is being raised in debug log (DCNE-677)

3 participants