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

Fix deployment list #3543

Merged
merged 9 commits into from
Oct 29, 2024
Merged

Fix deployment list #3543

merged 9 commits into from
Oct 29, 2024

Conversation

Mahmoud-Emad
Copy link
Contributor

@Mahmoud-Emad Mahmoud-Emad commented Oct 21, 2024

Description

There is an issue with listing VMs due to the merge of the Caprover PR

Changes

  • Keep the worker listed if the user deletes only the leader.
  • Remove the 'Manage Workers' button if the listed VM is a worker.
  • Remove the 'View Admin Panel' button if the listed VM is a worker.
  • Check if the full deployment has a 'leader' instead.
  • Fix listing issues in the other solutions.
  • Update the mergeCaproverDeployment function.
  • Fix an issue with adding a worker on a deployed Caprover leader

Related Issues

Tested Scenarios

A list of scenarios tried to match the deliverables

To consider

Preliminary Checks:

  • Does it completely address the issue linked?
  • What about edge cases?
  • Does it meet the specified acceptance criteria?
  • Are there any unintended side effects?
  • Does the PR adhere to the team's coding conventions, style guides, and best practices?
  • Does it integrate well with existing features?
  • Does it impact the overall performance of the application?
  • Are there any bottlenecks or slowdowns?
  • Has it been optimized for efficiency?
  • Has it been adequately tested with unit, integration, and end-to-end tests?
  • Are there any known defects or issues?
  • Is the code well-documented?
  • Are changes to documentation reflected in the code?

UI Checks:

  • If a UI design is provided/ does it follow it?
  • Does every button work?
  • Is the data displayed logical? Is it what you expected?
  • Does every validation work?
  • Does this interface feel intuitive?
  • What about slow network? Offline?
  • What if the gridproxy/graphql/chain is failing?
  • What would a first time user have a hard time navigating here?

Code Quality Checks:

  • Code formatted/linted? Are there unused imports? .. etc
  • Is there redundant/repeated code?
  • Are there conditionals that are always true or always false?
  • Can we write this more concisely?
  • Can we reuse this code? If yes, where?
  • Will the changes be easy to maintain and update in the future?
  • Can this code become too complex to understand for other devs?
  • Can this code cause future integration problems?

Checklist

  • Tests included
  • Build pass
  • Documentation
  • Code format and docstrings
  • Screenshots/Video attached (needed for UI changes)

- Keep the worker listed if the user deletes only the leader.
- Remove the 'Manage Workers' button if the listed VM is a worker.
- Remove the 'View Admin Panel' button if the listed VM is a worker.
- Check if the full deployment has a 'leader' instead.
- Fix listing issue in the other solutions.
- Update the mergeCaproverDeployment function.
- Fix an issue with adding a worker on a deployed Caprover leader
@Mahmoud-Emad Mahmoud-Emad marked this pull request as ready for review October 22, 2024 11:08
@Mahmoud-Emad Mahmoud-Emad changed the title fix: Fix deployment list Fix deployment list Oct 22, 2024
@Mahmoud-Emad
Copy link
Contributor Author

Tested scenarios

  • Listing in the Micro/Full VMs solutions
  • Listing a valid Caprover cluster
  • Listing an invalid Caprover cluster see the attached image

image

@Mahmoud-Emad Mahmoud-Emad self-assigned this Oct 24, 2024
@Mahmoud-Emad Mahmoud-Emad added this to the 2.6.0 milestone Oct 24, 2024
@Mahmoud-Emad Mahmoud-Emad marked this pull request as draft October 24, 2024 14:13
@Mahmoud-Emad
Copy link
Contributor Author

Converted to draft due to an issue happened when deleting the instances

- Replaced `Promise.all` with a `.reduce()` approach to sequentially await each `deleteDeployment` and `deleteGatewayDeployment` call.
@Mahmoud-Emad
Copy link
Contributor Author

Converted to draft due to an issue happened when deleting the instances

Should be fixed now

Investigation and Solution:

This issue might be due to how Promise.all handles errors versus the sequential for loop. With Promise.all, all deleteDeployment calls are executed concurrently, meaning the delete_machine method might check for the deployment existence before another instance of it finishes deleting. In contrast, a regular for loop executes sequentially, so each deleteDeployment call completes before the next starts, avoiding potential race conditions.

Work Completed:

Prevent a race condition in deployment deletion by switching to sequential execution

  • Replaced Promise.all with a .reduce() approach to sequentially await each deleteDeployment and deleteGatewayDeployment call.

@Mahmoud-Emad Mahmoud-Emad marked this pull request as ready for review October 27, 2024 07:31
Copy link
Contributor

@0oM4R 0oM4R left a comment

Choose a reason for hiding this comment

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

Screenshot from 2024-10-27 12-03-04
Screenshot from 2024-10-27 12-02-25

caprover deployments didn't show up on the deployment list, they were listed but after click on delete they are still on the contracts list,

@samaradel
Copy link
Contributor

I faced the same delete issue again
image

@0oM4R 0oM4R requested a review from samaradel October 29, 2024 09:03
@0oM4R
Copy link
Contributor

0oM4R commented Oct 29, 2024

Screenshot from 2024-10-27 12-03-04 Screenshot from 2024-10-27 12-02-25

caprover deployments didn't show up on the deployment list, they were listed but after click on delete they are still on the contracts list,

still happening..

Screenshot from 2024-10-29 11-59-38

also while deleting the cluster it shows 5 chips while i only have 1 leader and 2 workers
image
Screenshot from 2024-10-29 11-45-37

will ignore the caprover for now

Copy link
Contributor

@0oM4R 0oM4R left a comment

Choose a reason for hiding this comment

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

listing deployments works fine.
will reopen caprover issue
and will open new issue to revisit deployment list changes

@Mahmoud-Emad Mahmoud-Emad merged commit 0647659 into development Oct 29, 2024
10 checks passed
@Mahmoud-Emad Mahmoud-Emad deleted the development_fix_listing branch October 29, 2024 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants