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-8378] - OBJ Cleanup #10857

Merged
merged 8 commits into from
Sep 3, 2024

Conversation

jaalah-akamai
Copy link
Contributor

@jaalah-akamai jaalah-akamai commented Aug 29, 2024

Related

Changes 🔄

  • Removed our react hook form logic for BucketRateLimitTable and corresponding files. This is far from being ready and it introduced complexity since this component is used in 3 different places. This will most likely be a 2025 initiative to add this logic in when it's ready.
    • By removing this, we fixed an issue where app was crashing when trying to display the bucket rate table in any of the drawers for E2/E3.
  • Moved the "Bucket Rate Limits" title and corresponding helper text into BucketRateLimitTable.tsx since we were repeating that in 3 different places.
  • The Create Bucket drawer now displays each additional step once we have the preceding information selected.
    • Once we select a region, we'll show endpoint types
    • Once we select endpoint types, we'll show bucket rate limits (except for E0/E1 of course)

Target release date 🗓️

N/A

How to test 🧪

Prerequisites

  • Enable multi-cluster and gen2 flags
  • Enable MSW

Reproduction steps

(How to reproduce the issue, if applicable)

  • When selecting a E2/E3 endpoint type bucket, app would crash because we didn't implement our form logic to BucketDetailsDrawer

Verification steps

(How to verify changes)

  • Pull down branch or preview link

  • Select "Details" for any E2/E3 endpoint bucket:

    • Drawer should open and bucket rate limits table should show
  • Create a bucket and observe:

    • Once a region is selected, endpoint types show
    • Once we select endpoint types, bucket rate limits show (except for E0/E1 of course)
  • Go to bucket details page and then properties tab

    • Ensure title, description, and table all look good
    • Do this for E0 -> E3 endpoint types

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

@jaalah-akamai jaalah-akamai changed the title upcoming: [M3-8378] - Add form logic to bucket details for bucket rat… upcoming: [M3-8378] - OBJ Cleanup Aug 29, 2024
@jaalah-akamai jaalah-akamai marked this pull request as ready for review August 30, 2024 00:51
@jaalah-akamai jaalah-akamai requested a review from a team as a code owner August 30, 2024 00:51
@jaalah-akamai jaalah-akamai requested review from mjac0bs and abailly-akamai and removed request for a team August 30, 2024 00:51
Copy link
Contributor

@pmakode-akamai pmakode-akamai left a comment

Choose a reason for hiding this comment

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

Nice work with the clean-up 👍. Good catch on removing the react-hook-form logic for now.

  • Removed react-hook-form logic as planned ✅
  • No crash on details/create drawer page ✅
  • Rate limit table visible only for E2 and E3 endpoint types ✅
  • The Create Bucket drawer now displays each additional step once we have the preceding information selected ✅

Apart from the above, I observed a strange behavior: when the OBJ Gen 2 flag is enabled, reloading the application while on the SSL tab or Properties tab doesn't load the data (with mocked or real data).

I noticed:

const { data: bucketsData } = useObjectStorageBuckets(isObjectStorageGen2Enabled);

in BucketDetail/index.tsx at Line 53 returns undefined when isObjectStorageGen2Enabled is explicitly set after reloading the application on those tabs . The function defaults to true if the flag is not passed, and it works fine in that case. I’m not sure if others are observing the same behavior or if it’s due to account capabilities for me.

Screenshot 2024-08-30 at 4 21 54 PM

Screenshot 2024-08-30 at 4 21 01 PM

And the same in the case of Properties tab, too.

@coliu-akamai
Copy link
Contributor

@jaalah-akamai - looks like we caught the same bug 😆 Oops haha, going to close my PR, and could you unskip the BucketRateLimitTable tests in your PR? I'd previously had to skip them due to the bug

@jaalah-akamai
Copy link
Contributor Author

jaalah-akamai commented Aug 30, 2024

Apart from the above, I observed a strange behavior: when the OBJ Gen 2 flag is enabled, reloading the application while on the SSL tab or Properties tab doesn't load the data (with mocked or real data).

Yea that's expected because there's no real data yet, it's all mocked. Also when you refresh on the tab, our mocked bucket labels and endpoint_types change since they're randomized and the bucket you were looking at no longer exists. See serverHandlers.ts#L979-L985

Copy link

github-actions bot commented Aug 30, 2024

Coverage Report:
Base Coverage: 86.15%
Current Coverage: 86.15%

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.

Confirmed:

✅ When selecting "Details" for any E2/E3 endpoint bucket:

  • Drawer should open and bucket rate limits table should show

✅ When creating a bucket:

  • Once a region is selected, endpoint types show
  • Once we select endpoint types, bucket rate limits show (except for E0/E1 of course)

❓ On the bucket details page Properties tab for E0-E3 endpoint types:

  • Ensure title, description, and table all look good

This is what I saw on the Properties tab of E0 and E1 buckets. There seems to be a Save button, although there's nothing to Save. Is that unexpected? (The drawer looks fine.)

Tab Drawer
Screenshot 2024-08-30 at 7 48 09 AM Screenshot 2024-08-30 at 7 48 00 AM

) : (
'This endpoint type supports up to 750 Requests Per Second (RPS). '
)}
Understand <Link to="#">bucket rate limits</Link>.
Copy link
Contributor

@mjac0bs mjac0bs Aug 30, 2024

Choose a reason for hiding this comment

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

Two questions for my own knowledge, not directly related to these PR's changes:

  1. Have we moved away from using "Learn more about [topic]." in our copy? This is the first time I remember seeing "Understand [topic]." and it's used multiple times. I was wondering whether that's intentional or due to some inconsistency in tech writing.

Screenshot 2024-08-30 at 7 44 50 AM

  1. Why is E1 listed twice in this dropdown?
    Screenshot 2024-08-30 at 7 44 59 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I'll confirm with TW
  2. In rare cases, the dropdown must display a specific endpoint hostname (s3_endpoint) along with the endpoint type to distinguish between two assigned endpoints of the same type. This is necessary for multiple Gen1 (E1) assignments in the same region. Essentially, this mock demonstrates that if an s3_endpoint is provided, we want to display it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TW wants to keep it as is even though we use it in one other place

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for confirming and for the dropdown explanation. Any verdict on the Save button?

Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

I am lacking some context about the feature but PR is doing what it describes and fix the issue mentioned ✅

@jaalah-akamai jaalah-akamai merged commit da1ccfb into linode:develop Sep 3, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants