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-7423] - Create Bucket - Able to choose a single Compute Region ID eg ('us-east') #9976

Merged
merged 23 commits into from
Jan 3, 2024

Conversation

cpathipa
Copy link
Contributor

@cpathipa cpathipa commented Dec 7, 2023

Description 📝

OBJ Multi-Cluster - Create Bucket flow - Able to choose a single Compute Region ID eg ('us-east') instead of cluster ID.

Before After
image image

How to test 🧪

Prerequisites

(How to setup test environment)

Verification steps

(How to verify changes)

  • Verify there is no regression in existing functionality (Create Bucket flow with feature flag disabled and disable MSW).
  • Verify the new GUI as per the mockup.
  • Verify Create Bucket payload and response wrt to API spec.

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 review from a team as code owners December 7, 2023 17:54
@cpathipa cpathipa requested review from jdamore-linode, hana-akamai and abailly-akamai and removed request for a team December 7, 2023 17:55
@cpathipa cpathipa marked this pull request as draft December 7, 2023 17:55
@cpathipa cpathipa self-assigned this Dec 7, 2023
Copy link

github-actions bot commented Dec 7, 2023

Coverage Report:
Base Coverage: 79.25%
Current Coverage: 79.14%

@cpathipa cpathipa changed the base branch from feature/obj-multi-cluster to develop December 12, 2023 20:17
@cpathipa cpathipa marked this pull request as ready for review December 14, 2023 18:50
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.

Feature flag off & MSW disabled: existing functionality works as expected ✅

Feature flag on & MSW enabled: payload includes region instead of cluster and submission goes through ✅

Formatting of region options matches wireframes ✅
(did you want to update the copy for the Create Bucket drawer as part of this PR too?)

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.

Looks good!

I tested the flow and did not find issues with payload and UI. Agreed with comments already added and left some other.

Also, you have duplicated keys console errors when opening the drawer with MSW enabled. It's previous to the PR and not affecting production code, but can be a bit misleading when testing so it would be nice to fix

Screenshot 2023-12-15 at 10 10 27

packages/api-v4/src/object-storage/types.ts Outdated Show resolved Hide resolved
packages/manager/src/utilities/formatRegion.ts Outdated Show resolved Hide resolved
cpathipa and others added 11 commits December 15, 2023 09:40
Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>
…22373.md

Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>
…teBucketDrawerObjMultiCluster.tsx

Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>
…etRegions.tsx

Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>
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.

Thanks for addressing all the changes.

  • Code looks good ✅
  • Confirmed seeing the right regions in the UI ✅
  • Validation works as expected ✅

@cpathipa cpathipa added the Add'tl Approval Needed Waiting on another approval! label Dec 21, 2023
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 Chandra, this is looking good! Left one question related to required field and validation, slightly tangential to this PR.

  • Test are passing.
  • Create Bucket region dropdown now shows regions instead of clusters.
  • For regions with DC specific pricing, the overage costs are still correctly calculated.
  • Existing Create Bucket functionality remains unchanged when feature flag is off.
  • Mocked POST buckets submits and now includes the region rather than cluster.

Confirmed new POST:
Screenshot 2023-12-21 at 10 02 42 AM
Screenshot 2023-12-21 at 10 02 47 AM

packages/validation/src/buckets.schema.ts Show resolved Hide resolved
@mjac0bs mjac0bs added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! Ready for Review labels Dec 21, 2023
@cpathipa cpathipa merged commit 7246be9 into linode:develop Jan 3, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants