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.

Sign up for GitHub

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

Jump to bottom

Ha delete node for aws #7683

Merged
merged 14 commits into from
Feb 6, 2023
Merged

Ha delete node for aws #7683

merged 14 commits into from
Feb 6, 2023

Conversation

shaik80
Copy link
Member

@shaik80 shaik80 commented Jan 23, 2023 •
edited by vivekshankar1
Loading

🔩 Description: What code changed, and why?

⛓️ Related Resources

https://chefio.atlassian.net/browse/SHIELD-221
scenarios tried out:

  • removing node after fresh deploy
  • removing node after running add node
  • removing node after taking a backup

👍 Definition of Done

👟 How to Build and Test the Change

✅ Checklist

All PRs must tick these:

With occasional exceptions, all PRs from Progress employees must tick these:

  • Is the code clear? (complicated code or lots of comments--subdivide and use well-named methods, meaningful variable names, etc.)
  • Consistency checked? (user notifications, user prompts, visual patterns, code patterns, variable names)
  • Repeated code blocks eliminated? (adapt and reuse existing components, blocks, functions, etc.)
  • Spelling, grammar, typos checked? (at a minimum use make spell in any component directory)
  • Code well-formatted? (indents, line breaks, etc. improve rather than hinder readability)

All PRs from Progress employees should tick these if appropriate:

  • Tests added/updated? (all new code needs new tests)
  • Docs added/updated? (all customer-facing changes)

Please add a note next to any checkbox above if you are NOT ticking it.

📷 Screenshots, if applicable

video:
https://progresssoftware.sharepoint.com/:v:/s/ChefCoreC/Ea3JkQpqUw5AobKSC8WdXVgBRxLQuIZRCMhSm7bZmAlDrg?e=C6GzPI
Screenshot 2023-02-02 at 7 48 29 PM

@shaik80 shaik80 requested a review from a team as a code owner January 23, 2023 13:45
@netlify
Copy link

netlify bot commented Jan 23, 2023 •
edited
Loading

✅ Deploy Preview for chef-automate ready!

Name Link
🔨 Latest commit 6a19edc
🔍 Latest deploy log https://app.netlify.com/sites/chef-automate/deploys/63e129d3cb93c50008a9fbd6
😎 Deploy Preview https://deploy-preview-7683--chef-automate.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions github-actions bot added the documentation Anything related to the Automate docs. label Jan 23, 2023
@github-actions github-actions bot removed the documentation Anything related to the Automate docs. label Jan 23, 2023
@shaik80 shaik80 changed the title [wip] Ha delete node for aws Ha delete node for aws Jan 27, 2023
@shaik80 shaik80 self-assigned this Jan 27, 2023
@github-actions github-actions bot added the documentation Anything related to the Automate docs. label Jan 27, 2023
punitmundra
punitmundra reviewed Feb 3, 2023
View reviewed changes
components/automate-cli/cmd/chef-automate/ha_node_delete_aws.go
}

argsdeploy := []string{"-y"}
err = dna.nodeUtils.executeAutomateClusterCtlCommandAsync("provision", argsdeploy, provisionInfraHelpDocs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

In case of Provision Infra fails for any reason =, then we do not have flow for that

punitmundra
punitmundra approved these changes Feb 3, 2023
View reviewed changes
Copy link
Collaborator

@punitmundra punitmundra left a comment

Choose a reason for hiding this comment

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

LGTM, but later we have to handle the flow in which provision command fails, then we have ..to go backup to the previous state.

Dmaddu
Dmaddu requested changes Feb 3, 2023
View reviewed changes
Copy link
Collaborator

@Dmaddu Dmaddu left a comment

Choose a reason for hiding this comment

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

only one change, rest looks good to me

components/automate-cli/cmd/chef-automate/ha_node_delete_aws.go Outdated

func (dna *DeleteNodeAWSImpl) runRemoveNodeFromAws() error {
if len(dna.automateIpList) == 1 {
for i := 0; i < len(dna.configAutomateIpList); i++ {
Copy link
Collaborator

Choose a reason for hiding this comment

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

From line number 195 - 204, the same logic we are doing for all 4 types, can we write a common code instead of repeating the code?

Copy link
Collaborator

Choose a reason for hiding this comment

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

done

shaik80 and others added 7 commits February 6, 2023 21:53
@shaik80 @vivekshankar1
aws node delete init
937afb6
@shaik80 @vivekshankar1
working code to remove aws node
4f5efb2 
Signed-off-by: shaik80 <sh.mudassir98@gmail.com>
@shaik80 @vivekshankar1
add unit test cases and simplify code
c1818e3 
Signed-off-by: shaik80 <sh.mudassir98@gmail.com>
@shaik80 @vivekshankar1
lint fix
c805caf 
Signed-off-by: shaik80 <sh.mudassir98@gmail.com>
@shaik80 @vivekshankar1
doc changes
e32eebf 
Signed-off-by: shaik80 <sh.mudassir98@gmail.com>
@vivekshankar1
add test case
163c782 
Signed-off-by: Vivek Shankar <vshankar@progress.com>
@vivekshankar1
add test cases
55e3c1b 
Signed-off-by: Vivek Shankar <vshankar@progress.com>
@vivekshankar1
some refactoring
b128795 
Signed-off-by: Vivek Shankar <vshankar@progress.com>
@vivekshankar1
improved err text
cca804c 
Signed-off-by: Vivek Shankar <vshankar@progress.com>
@vivekshankar1
warning for remove node
655ff64 
Signed-off-by: Vivek Shankar <vshankar@progress.com>
@vivekshankar1
change cli output
1805f69 
Signed-off-by: Vivek Shankar <vshankar@progress.com>
@vivekshankar1
refactored code
927de4c 
Signed-off-by: Vivek Shankar <vshankar@progress.com>
@vivekshankar1
improved code
ed45056 
Signed-off-by: Vivek Shankar <vshankar@progress.com>
@vivekshankar1
improved readme text
6a19edc 
Signed-off-by: Vivek Shankar <vshankar@progress.com>
@vivek-yadav vivek-yadav merged commit 4abfa28 into main Feb 6, 2023
@vivek-yadav vivek-yadav deleted the ha_delete_node_for_aws branch February 6, 2023 16:26
@sonarcloud
Copy link

sonarcloud bot commented Feb 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 107 Security Hotspots
Code Smell A 18 Code Smells

81.1% 81.1% Coverage
45.3% 45.3% Duplication

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

@vivek-yadav vivek-yadav vivek-yadav approved these changes

@atultherajput atultherajput atultherajput approved these changes

@punitmundra punitmundra punitmundra approved these changes

@Dmaddu Dmaddu Dmaddu approved these changes

@vivekshankar1 vivekshankar1 vivekshankar1 approved these changes

@prasad927 prasad927 Awaiting requested review from prasad927

Assignees

@shaik80 shaik80

Labels
documentation Anything related to the Automate docs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@shaik80 @vivek-yadav @atultherajput @punitmundra @Dmaddu @vivekshankar1