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

remove v1 monitoring code #10539

Merged

Conversation

aalves08
Copy link
Contributor

@aalves08 aalves08 commented Feb 29, 2024

Summary

Fixes #10490

Occurred changes and/or fixed issues

  • remove v1 monitoring code
  • add e2e tests

Technical notes summary

  • Nothing noteworthy

Areas or cases that should be tested

Areas which could experience regressions

Make sure we cover the areas mentioned in the previous points to avoid regressions. Manual testing might be needed to ensure V2 monitoring is working fine

Screenshot/Video

Screen.Recording.2024-03-11.at.11.17.17.mov

Checklist

  • The PR is linked to an issue and the linked issue has a Milestone, or no issue is needed
  • The PR has a Milestone
  • The PR template has been filled out
  • The PR has been self reviewed
  • The PR has a reviewer assigned
  • The PR has automated tests or clear instructions for manual tests and the linked issue has appropriate QA labels, or tests are not needed
  • The PR has reviewed with UX and tested in light and dark mode, or there are no UX changes

@aalves08 aalves08 added this to the v2.9.0 milestone Feb 29, 2024
@aalves08 aalves08 self-assigned this Feb 29, 2024
@aalves08
Copy link
Contributor Author

@nwmac @richard-cox added you both as reviewers since this need a keen eye. I went to each of those views and all looked good.
Not fully sure if the uninstall can all go away, but I believe I captured the correct code removal. Didn't test this... Is it testable even?

Copy link
Member

@richard-cox richard-cox left a comment

Choose a reason for hiding this comment

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

There's a couple of items in the checklist that still need doing

The PR template has been filled out
The PR has automated tests or clear instructions for manual tests and the linked issue has appropriate QA labels, or tests are not needed

Automated test side, we probably need some to cover below. Hopefully all have existing tests though

  • Node list --> click on one --> confirm something trivial on page
  • Workload list --> click on one --> confirm something trivial on page
  • Cluster Dashboard --> confirm something trivial on page

Monitoring is a bit more of a pain, would need some manual tests for that (it's a honker to install on the small image used by gh ci)

shell/chart/monitoring/steps/uninstall-v1.vue Outdated Show resolved Hide resolved
@aalves08
Copy link
Contributor Author

@richard-cox the issue has QA/None label... Should I change it to QA/manual-test?

Alexandre Alves added 3 commits March 22, 2024 10:18
… strings + simple checks for detail pages in e2e tests to verify integrition of work done
@aalves08 aalves08 force-pushed the 10490-remove-v1-monitoring-embedded-pages branch from 2d92165 to 5882ab0 Compare March 22, 2024 10:18
@aalves08 aalves08 merged commit 5409f2b into rancher:master Mar 22, 2024
24 checks passed
codyrancher pushed a commit to codyrancher/dashboard that referenced this pull request Apr 20, 2024
* remove v1 monitoring code

* fix lint issue

* remove monitoring v1 uninstall code + remove v1 uninstall translation strings + simple checks for detail pages in e2e tests to verify integrition of work done

* address pr comments

* fix unit tests

---------

Co-authored-by: Alexandre Alves <aalves@Alexandres-MacBook-Pro.local>
codyrancher pushed a commit to codyrancher/dashboard that referenced this pull request Apr 24, 2024
* remove v1 monitoring code

* fix lint issue

* remove monitoring v1 uninstall code + remove v1 uninstall translation strings + simple checks for detail pages in e2e tests to verify integrition of work done

* address pr comments

* fix unit tests

---------

Co-authored-by: Alexandre Alves <aalves@Alexandres-MacBook-Pro.local>
codyrancher added a commit that referenced this pull request Apr 29, 2024
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.

Remove v1 monitoring embedded pages
2 participants