Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bugfix] Resolves deletion of nodes from controller upon destroy (DCNE-152) #1266

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

anvitha-jain
Copy link
Collaborator

solves #1214

@codecov-commenter
Copy link

codecov-commenter commented Aug 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.25%. Comparing base (f95173e) to head (df3bc26).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1266   +/-   ##
=======================================
  Coverage   85.25%   85.25%           
=======================================
  Files         102      102           
  Lines       37812    37812           
=======================================
  Hits        32238    32238           
  Misses       4118     4118           
  Partials     1456     1456           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@samiib samiib changed the title [bugfix] Resolves deletion of nodes from controller upon destroy [bugfix] Resolves deletion of nodes from controller upon destroy (DCNE-152) Aug 14, 2024
Copy link
Collaborator

@gmicol gmicol left a comment

Choose a reason for hiding this comment

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

need to fix CI failure

aci/resource_aci_fabricnodeidentp.go Outdated Show resolved Hide resolved
@anvitha-jain anvitha-jain added the jira-sync Sync this issue to Jira label Aug 16, 2024
@github-actions github-actions bot changed the title [bugfix] Resolves deletion of nodes from controller upon destroy (DCNE-152) [bugfix] Resolves deletion of nodes from controller upon destroy (DCNE-152) (DCNE-159) Aug 20, 2024
samiib
samiib previously approved these changes Aug 20, 2024
Copy link
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

aci/resource_aci_fabricnodeidentp.go Show resolved Hide resolved
aci/resource_aci_fabricnodeidentp.go Outdated Show resolved Hide resolved
aci/resource_aci_fabricnodeidentp.go Show resolved Hide resolved
aci/resource_aci_fabricnodeidentp.go Outdated Show resolved Hide resolved
@samiib samiib removed the jira-sync Sync this issue to Jira label Aug 22, 2024
@samiib samiib changed the title [bugfix] Resolves deletion of nodes from controller upon destroy (DCNE-152) (DCNE-159) [bugfix] Resolves deletion of nodes from controller upon destroy (DCNE-152) Aug 22, 2024
samiib
samiib previously approved these changes Aug 22, 2024
Copy link
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

shrsr
shrsr previously approved these changes Sep 9, 2024
Copy link
Collaborator

@shrsr shrsr left a comment

Choose a reason for hiding this comment

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

LGTM

samiib
samiib previously approved these changes Sep 9, 2024
Copy link
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


"commission": &schema.Schema{
Type: schema.TypeString,
Optional: true,
Copy link
Collaborator

@akinross akinross Sep 10, 2024

Choose a reason for hiding this comment

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

Ok but this is not part of the DN right? In what scenario would this be provided by a user when it gets read and set in the getAndsetDecommissionedNodes function?

Changed

docs/data-sources/fabric_node_member.md Outdated Show resolved Hide resolved
docs/resources/fabric_node_member.md Outdated Show resolved Hide resolved
docs/data-sources/fabric_node_member.md Show resolved Hide resolved

"commission": &schema.Schema{
Type: schema.TypeString,
Optional: true,
Copy link
Collaborator

@akinross akinross Sep 11, 2024

Choose a reason for hiding this comment

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

could you provide explanation why?

removed optional, for commission and for the rest changes are not made as discussed

legacy-docs/docs/r/fabric_node_member.html.markdown Outdated Show resolved Hide resolved
legacy-docs/docs/r/fabric_node_member.html.markdown Outdated Show resolved Hide resolved
samiib
samiib previously approved these changes Sep 12, 2024
Copy link
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

legacy-docs/docs/d/fabric_node_member.html.markdown Outdated Show resolved Hide resolved
legacy-docs/docs/r/fabric_node_member.html.markdown Outdated Show resolved Hide resolved
legacy-docs/docs/r/fabric_node_member.html.markdown Outdated Show resolved Hide resolved
aci/resource_aci_fabricnodeidentp.go Outdated Show resolved Hide resolved
aci/resource_aci_fabricnodeidentp.go Outdated Show resolved Hide resolved

log.Printf("[DEBUG] %s: Destroy finished successfully", d.Id())

d.SetId("")
return diag.FromErr(err)
}
func verifyNodeAttachedToSwitch(aciClient *client.Client, serial string) (bool, error) {
switchStatusDn := fmt.Sprintf("client-[%s]", serial)
Copy link
Collaborator

Choose a reason for hiding this comment

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

see code below where I think it is possible to log the end of this and thus to outcome of the function

aci/resource_aci_fabricnodeidentp.go Show resolved Hide resolved

### Read-Only ###

* `id` - (string) The distinguished name (DN) of the Fabric Node Member object.
Copy link
Collaborator

Choose a reason for hiding this comment

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

object

- `pod_id` - (Optional) The pod id of the new Fabric Node Member. Allowed value range: "1" - "254". Default value: "1".
- `role` - (Optional) Role for the new Fabric Node Member.
Allowed values: "unspecified", "leaf", "spine". Default value: "unspecified".
* `annotation` (annotation) - (string) Specifies the annotation of a Fabric Node Member.
Copy link
Collaborator

Choose a reason for hiding this comment

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

no object

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aci_fabric_node_member resource do not remove node from inventory (DCNE-152)
8 participants