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: CapRover Dashboard App: Details of Workers disappear after few seconds #3363

Merged
merged 18 commits into from
Oct 20, 2024

Conversation

Mahmoud-Emad
Copy link
Contributor

@Mahmoud-Emad Mahmoud-Emad commented Sep 3, 2024

Description

Fix listing Caprover workers

Changes

  • Send both of the deployments, leader, and workers.
  • Call the 'getGrafanaUrl' on mount the component.
  • Delete the full cluster.
  • Ensure that workers are correctly removed from the deployment list when the leader is deleted to maintain data integrity.
  • Merged the latest changes from the development branch to keep the codebase up to date.
  • Conducted thorough testing to verify the complete functionality and ensure no regressions were introduced.

Test Scenarios

  1. Deploy leader, and worker, Go go to the contracts page and delete the deployed leader.
  2. Deploy leader, attach a worker to it then delete the whole cluster just in time without refreshing the page.
  3. Deploy leader, and worker, delete the deployed worker and attach another, then delete the whole cluster just in time without refreshing the page.
  4. Deploy a cluster with 1 leader and 2 workers check the deployment listing dialog and make sure all of them are listed
  5. Delete a cluster with the leader and more than 3 workers and make sure all of them are deleted.

PS: Make sure no errors on the Caprover listing page.

Related Issues

Screenshots/Videos

Screencast.from.09-03-2024.05.27.14.PM.webm

image

Checklist

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

- Enhanced Deletion Dialog: displays all attached workers in the deletion dialog to prompt user confirmation before proceeding.
- Worker Deletion Logic: Iterates through the machine's interface, extracts the associated worker nodes, and deletes them efficiently.
- Refactored Delete Function: Improved the delete function.
@Mahmoud-Emad Mahmoud-Emad marked this pull request as ready for review September 4, 2024 07:44
- Used the some() method since it'll be faster than the `filter` method.
- Update the checker to check if the selected items have workers.
maayarosama
maayarosama previously approved these changes Sep 9, 2024
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.

I was able to list and delete all workers fine, but on adding new one i got this error
Screenshot from 2024-09-12 12-06-51

@Mahmoud-Emad
Copy link
Contributor Author

I was able to list and delete all workers fine, but on adding new one i got this error Screenshot from 2024-09-12 12-06-51

This happened because the worker is listed instead of the master, which is wrong

@Mahmoud-Emad Mahmoud-Emad marked this pull request as draft September 16, 2024 06:59
- Push the master into the first index of the result array
- Push the workers into the master, second index of the result array
- Tested the code well
@Mahmoud-Emad Mahmoud-Emad marked this pull request as ready for review September 22, 2024 09:40
Copy link
Contributor

@zaelgohary zaelgohary left a comment

Choose a reason for hiding this comment

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

Now listing works fine as it lists the leader not the worker, however, on adding another worker after deploying, I faced this error.

image

@Mahmoud-Emad
Copy link
Contributor Author

Mahmoud-Emad commented Sep 24, 2024

Now listing works fine as it lists the leader not the worker, however, on adding another worker after deploying, I faced this error.

image

This issue isn't related to my PR, it could be another issue, it says there is another network workload with the same name already exists.

@zaelgohary
Copy link
Contributor

Workers are being showed as leaders in contracts page.

image

image

image

@zaelgohary
Copy link
Contributor

I deleted the caprover deployment but 1 network contract still exist after deletion. Happened more than once.

image

@AhmedHanafy725
Copy link
Contributor

Workers are being showed as leaders in contracts page.

image

image

image

@zaelgohary In the contracts, all the cluster's workers carry the same name as the cluster (which is the leader name) so we can list them as part of the cluster

@Mahmoud-Emad
Copy link
Contributor Author

I deleted the caprover deployment but 1 network contract still exist after deletion. Happened more than once.

image

It's a known issue i think #3425

@Mahmoud-Emad Mahmoud-Emad added this to the 2.6.0 milestone Sep 26, 2024
@Mahmoud-Emad Mahmoud-Emad self-assigned this Sep 26, 2024
@zaelgohary
Copy link
Contributor

zaelgohary commented Sep 26, 2024

I deleted the caprover deployment but 1 network contract still exist after deletion. Happened more than once.
image

It's a known issue i think #3425

It's not. It was fixed in #3456 which was merged yesterday. Can you merge development to your branch? and if the issue still exist, we can open an issue for it as this only happen in Caprover.

@Mahmoud-Emad
Copy link
Contributor Author

The issue is already solved yeah, I just noticed that I have to build the client first

image

However, yeah one contract still exists and should be deleted!

#3473

Copy link
Contributor

@zaelgohary zaelgohary left a comment

Choose a reason for hiding this comment

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

I tried listing a deployment I have but got the following errors. Am I doing anything wrong?

image

image

@zaelgohary
Copy link
Contributor

I deployed a new instance & whenever I view its details I get the following error:

image

I also got the same error when deleting a worker.

@maayarosama
Copy link
Contributor

I tried adding workers most of the times it passed, but it failed once. Deletion works fine
Screenshot from 2024-09-29 13-22-25
Screenshot from 2024-09-29 13-24-07

@Mahmoud-Emad Mahmoud-Emad marked this pull request as draft October 1, 2024 06:44
- Ensure that workers are correctly removed from the deployment list when the leader is deleted to maintain data integrity.
- Merged the latest changes from the development branch to keep the codebase up to date.
- Conducted thorough testing to verify the complete functionality and ensure no regressions were introduced.

Test Scenarios

1. Deploy `leader`, and `worker`, Go go to the contracts page and delete the deployed `leader`.
2. Deploy `leader`, attach a `worker` to it then delete the whole cluster just in time without refreshing the page.
3. Deploy `leader`, and `worker`, delete the deployed `worker` and attach another, then delete the whole cluster just in time without refreshing the page.
4. Deploy a cluster with 1 leader and 2 workers check the deployment listing dialog and make sure all of them are listed
3. Delete a cluster with the leader and more than 3 workers and make sure all of them are deleted.

PS: Make sure no errors on the Caprover listing page.
@Mahmoud-Emad Mahmoud-Emad marked this pull request as ready for review October 17, 2024 11:19
@Mahmoud-Emad Mahmoud-Emad removed the request for review from mohamedamer453 October 17, 2024 11:23
Copy link
Contributor

@zaelgohary zaelgohary left a comment

Choose a reason for hiding this comment

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

Works fine adding/deleting clusters & works.

image

@Mahmoud-Emad Mahmoud-Emad merged commit d1f7304 into development Oct 20, 2024
10 checks passed
@Mahmoud-Emad Mahmoud-Emad deleted the development_caprover branch October 20, 2024 09:09
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.

6 participants