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.

Sign up for GitHub

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

Jump to bottom

upcoming: [M3-7743] - Handle errors gracefully when FF enabled without MSW #10189

Merged
merged 11 commits into from
Feb 21, 2024
Merged

upcoming: [M3-7743] - Handle errors gracefully when FF enabled without MSW #10189

merged 11 commits into from
Feb 21, 2024

Conversation

cpathipa
Copy link
Contributor

@cpathipa cpathipa commented Feb 13, 2024 •
edited
Loading

Description 📝

Handles error scenarios gracefully when MSW is off.

Changes 🔄

List any change relevant to the reviewer.

  • Code cleanup AccessKeyTableRow
  • Decoupled AccessKeyTableRow
  • Decoupled HostNameTableCell

How to test 🧪

Prerequisites

(How to setup test environment)

  • Turn on OBJ Multi Cluster FF
  • Turn OFF MSW

Reproduction steps

(How to reproduce the issue, if applicable)

  • Checkout to develop turn on OBJ Multi Cluster FF and MSW OFF.
  • Navigate to Accesskey landing page.
  • Should notice app crashing.

Verification steps

(How to verify changes)

  • Ensure APP is not crashing when FF is turned on in Object storage flow, like Create Bucket, Create / update Access key, Access key landing page etc.
  • Turn off the feature flag and ensure there is no regression in object storage flow.

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 February 13, 2024 15:29
@cpathipa cpathipa requested review from dwiley-akamai and hana-akamai and removed request for a team February 13, 2024 15:29
@cpathipa cpathipa marked this pull request as draft February 13, 2024 15:30
@cpathipa cpathipa self-assigned this Feb 13, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 13, 2024 •
edited
Loading

Coverage Report: ✅
Base Coverage: 81.33%
Current Coverage: 81.33%

@cpathipa cpathipa marked this pull request as ready for review February 13, 2024 22:53
cpathipa and others added 2 commits February 14, 2024 12:51
@cpathipa @dwiley-akamai
Update packages/manager/.changeset/pr-10189-upcoming-features-1707838…
63cd0d4 
…240012.md

Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>
@cpathipa
PR - feedback @dwiley-akamai
992c404
@cpathipa cpathipa mentioned this pull request Feb 15, 2024
Merged
12 tasks
dwiley-akamai
dwiley-akamai approved these changes Feb 16, 2024
View reviewed changes
Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

App doesn't crash with feature flag enabled but MSW off ✅
Didn't observe any regressions in normal flows with feature flag off ✅

I would still prefer doing away with the intermediary AccessKeyTableRows.tsx file since that departs from how we handle tables/rows elsewhere in the app, but approving given the above and since this PR didn't introduce that component.

packages/manager/src/features/Profile/SecretTokenDialog/SecretTokenDialog.tsx
Service Worker (MSW). This can be removed during the feature flag cleanup. */}
{isObjMultiClusterEnabled &&
objectStorageKey &&
objectStorageKey?.regions?.length > 0 && (
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe store in a variable since it gets used twice

@cpathipa
Copy link
Contributor Author

AccessKeyTableRows
@dwiley-akamai I see what you mean: eliminate AccessKeyTableRows and loop through AccessKeyTableRow within the AccessKeyTableBody, which we could definitely do. I thought we would keep the AccessKeyTableRow logic in AccessKeyTableBody.

@jaalah-akamai
Copy link
Contributor

cc: @hana-linode for review

bnussman-akamai
bnussman-akamai approved these changes Feb 20, 2024
View reviewed changes
Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

Only issue I see is this when the Multi-Cluster feature flag is on

Screenshot 2024-02-20 at 10 23 39 AM

hana-akamai
hana-akamai approved these changes Feb 20, 2024
View reviewed changes
Copy link
Contributor

@hana-akamai hana-akamai left a comment

Choose a reason for hiding this comment

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

Confirmed that the app no longer crashes when the OBJ Multi Cluster feature flag is on with the MSW off

@hana-akamai hana-akamai added Approved Multiple approvals and ready to merge! and removed Ready for Review labels Feb 20, 2024
@cpathipa
Copy link
Contributor Author

Only issue I see is this when the Multi-Cluster feature flag is on

Screenshot 2024-02-20 at 10 23 39 AM

Now, we return an empty cell instead of null when there is no data(MSW off). This addresses the issue.. (
f5c8be6)

@cpathipa cpathipa merged commit b9819bb into linode:develop Feb 21, 2024
17 of 18 checks passed
@cpathipa cpathipa deleted the M3-7743 branch February 21, 2024 17:22
This was referenced Mar 1, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hana-akamai hana-akamai hana-akamai approved these changes

@bnussman-akamai bnussman-akamai bnussman-akamai approved these changes

@dwiley-akamai dwiley-akamai dwiley-akamai approved these changes

@jaalah-akamai jaalah-akamai Awaiting requested review from jaalah-akamai

Assignees

@cpathipa cpathipa

Labels
Approved Multiple approvals and ready to merge! OBJ Multi-Cluster
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cpathipa @jaalah-akamai @dwiley-akamai @bnussman-akamai @hana-akamai