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

upcoming: [M3-7798] - Update Buckets landing page to use regions instead of clusters. #10244

Merged
merged 15 commits into from
Mar 13, 2024

Conversation

cpathipa
Copy link
Contributor

@cpathipa cpathipa commented Mar 1, 2024

Description 📝

Update Buckets landing page to use regions instead of clusters.

Prerequisites

(How to setup test environment)

  • Turn on OBJ feature flag
  • Turn on MSW

Verification steps

(How to verify changes)

Buckets Landing page:

Bucket Details Drawer:

  • Navigate to http://localhost:3000/object-storage/buckets.
  • Click on Details action button and validate bucket access is fetched (endpoint) with regions instead of clusters.
  • Update the Details and verify the bucket access updated with region (endpoint) instead of clusters.

Turn off feature flag and MSW.

  • Ensure there is no regression in Buckets flow.
  • Ensure there is no regression Buckets landing page.
  • Ensure there is no regression buckets create flow.
  • Ensure there is no regression buckets details page (http://localhost:3000/object-storage/buckets/{cluster}/{bucket_name}).
  • Ensure there is no regression in view bucket details drawer.
  • Ensure there is no regression in update bucket details drawer.

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@cpathipa cpathipa requested a review from a team as a code owner March 1, 2024 15:36
@cpathipa cpathipa requested review from mjac0bs and carrillo-erik and removed request for a team March 1, 2024 15:36
@cpathipa cpathipa marked this pull request as draft March 1, 2024 15:37
@cpathipa cpathipa self-assigned this Mar 1, 2024
Copy link

github-actions bot commented Mar 1, 2024

Coverage Report:
Base Coverage: 81.39%
Current Coverage: 81.39%

@cpathipa cpathipa marked this pull request as ready for review March 1, 2024 23:00
@cpathipa cpathipa requested a review from dwiley-akamai March 1, 2024 23:01
Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

This was looking good from my testing, though typecheck is currently failing:

image

  • Validated buckets landing is fetched with regions instead of clusters (e.g. GET object-storage/buckets/us-iad?page_size=500)
    Note: I'm not very familiar with the Object-MultiCluster feature or the mock data, but in addition to the GET requests for buckets that use regions, I was also seeing requests that include the cluster. Wasn't certain if that was still expected.
    Screenshot 2024-03-04 at 10 32 58 AM

  • Validated bucket access is fetched (endpoint) with regions instead of clusters (e.g. GET object-storage/buckets/us-iad/obj-bucket-1/access)

  • Verified the bucket access PUT request has updated with region instead of clusters (e.g. PUT object-storage/buckets/us-iad/obj-bucket-1/access)

  • With the feature flag off, tested Buckets landing and Details CRUD flows for regressions

@carrillo-erik
Copy link
Contributor

I was able to verify that the buckets are fetched with regions instead of clusters and that bucket access is fetched with regions endpoint instead of clusters. Functionality appears to be working as expected.

One minor visual regression I noticed is with the carat (^) symbol in the dropdown menu. In other places in the app, when the user engages with the dropdown the carat symbol is inverted and the color changes from gray to blue, denoting it's active. That is not the case with this PR, please take a look at the photos below.

Current Behavior:

Screenshot 2024-03-05 at 12 26 00 PM

Expected Behavior:

Screenshot 2024-03-05 at 12 26 35 PM

@cpathipa
Copy link
Contributor Author

I was able to verify that the buckets are fetched with regions instead of clusters and that bucket access is fetched with regions endpoint instead of clusters. Functionality appears to be working as expected.

One minor visual regression I noticed is with the carat (^) symbol in the dropdown menu. In other places in the app, when the user engages with the dropdown the carat symbol is inverted and the color changes from gray to blue, denoting it's active. That is not the case with this PR, please take a look at the photos below.

Current Behavior:

Screenshot 2024-03-05 at 12 26 00 PM

Expected Behavior:

Screenshot 2024-03-05 at 12 26 35 PM

@carrillo-erik Good find. It looks like the same behavior is present in production as well, not introduced by this PR. I'm not sure what's causing this issue. Considering the scope of this ticket, I've created a separate bug ticket (M3-7864) to address this issue.
image

@cpathipa cpathipa requested a review from dwiley-akamai March 12, 2024 17:01
@cpathipa
Copy link
Contributor Author

cpathipa commented Mar 12, 2024

This was looking good from my testing, though typecheck is currently failing:

image

  • Validated buckets landing is fetched with regions instead of clusters (e.g. GET object-storage/buckets/us-iad?page_size=500)
    Note: I'm not very familiar with the Object-MultiCluster feature or the mock data, but in addition to the GET requests for buckets that use regions, I was also seeing requests that include the cluster. Wasn't certain if that was still expected.
    Screenshot 2024-03-04 at 10 32 58 AM
  • Validated bucket access is fetched (endpoint) with regions instead of clusters (e.g. GET object-storage/buckets/us-iad/obj-bucket-1/access)
  • Verified the bucket access PUT request has updated with region instead of clusters (e.g. PUT object-storage/buckets/us-iad/obj-bucket-1/access)
  • With the feature flag off, tested Buckets landing and Details CRUD flows for regressions

@mjac0bs Thank you for the thorough testing.

  • Addressed the typecheck in 53f796d
  • There are a couple of places in CM where we are fetching buckets with clusters, such as SearchBar.tsx, SearchLanding.tsx, and PrimaryNav.tsx. Considering current PR size, scope and testing efforts, I will create a follow-up PR as part of M3-7536 to address this.

@cpathipa cpathipa requested a review from mjac0bs March 12, 2024 17:52
@cpathipa cpathipa added the Add'tl Approval Needed Waiting on another approval! label Mar 12, 2024
@cpathipa
Copy link
Contributor Author

Looking into the failed object-storage.smoke.spec test

@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Mar 12, 2024
Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the feedback! Confirmed that other than the test failure, this all looks good.

@cpathipa cpathipa requested a review from a team as a code owner March 13, 2024 12:31
@cpathipa cpathipa requested review from jdamore-linode and removed request for a team March 13, 2024 12:31
Copy link
Contributor

@jdamore-linode jdamore-linode left a comment

Choose a reason for hiding this comment

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

Just want to call out an issue with the recent change to the e2e file before it gets merged! Happy to help work out this test issue

Copy link
Contributor

@jdamore-linode jdamore-linode left a comment

Choose a reason for hiding this comment

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

Thanks @cpathipa, looks good!

@cpathipa cpathipa merged commit 306868d into linode:develop Mar 13, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! OBJ Multi-Cluster Ready for Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants