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-7615] - Add placement group to payload #10195

Merged
merged 9 commits into from
Feb 20, 2024

Conversation

carrillo-erik
Copy link
Contributor

@carrillo-erik carrillo-erik commented Feb 14, 2024

Description 📝

The main concept with this PR is to include the details of the selected Placement Group as part of the Create Linode workflow.

Changes 🔄

List any change relevant to the reviewer.

  • The LabelAndTagsPanel has been renamed to DetailsPanel since the panel had been updated to include the PlacementGroupSelect component. Accordingly, the Details title has been added to the panel.
  • If the selected PlacementGroup does not have any capacity, an error Notice is displayed within the panel.
  • Updates to the Linodes schema to include Placement Group details.

Preview 📷

Details-Panel

Details-Panel-Error

How to test 🧪

(How to setup test environment)

  • Enable the following using the Cloud Manager Dev Tools:
    • Placement Groups feature flag
    • MSW

Verification steps

Success

  • Go through the Create Linode workflow and select Newark, NJ (us-east) for the Region.
  • Select any of the listed Placement Groups.
  • Complete the remaining fields and click on the Create Linode button.
  • Verify that the request was successful.

Error

  • Update the placementGroupFactory by adding an additional element to the linode_ids array. This will make the Placement Group be at full capacity.
  • Go through the Create Linode workflow as above.
  • Verify that clicking on the Create Linode button results in an error and the Notice is displayed.

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

@carrillo-erik carrillo-erik marked this pull request as ready for review February 16, 2024 15:58
@carrillo-erik carrillo-erik requested a review from a team as a code owner February 16, 2024 15:58
@carrillo-erik carrillo-erik requested review from hana-akamai and jaalah-akamai and removed request for a team February 16, 2024 15:58
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.

PR looks and UI look good, however we still need some changes

  1. The placement_group payload is still sent when the flag is OFF, which we want to avoid

  2. the validation does not seem to work properly, I was able to send a POST with just the strict value?
    Screenshot 2024-02-16 at 11 41 15

  3. You need to address those affinity labels
    305476889-3f420ded-f3b0-4335-9f18-c7242f7b54c2

4, left some comments to change the strict boolean in the payload

packages/api-v4/src/linodes/types.ts Outdated Show resolved Hide resolved
packages/validation/src/linodes.schema.ts Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Feb 16, 2024

Coverage Report:
Base Coverage: 81.24%
Current Coverage: 81.24%

@carrillo-erik
Copy link
Contributor Author

  1. The placement_group payload is still sent when the flag is OFF, which we want to avoid

@abailly-akamai
This makes sense, however, if the feature flag is OFF; should the value of placement_group be null or undefined?

@abailly-akamai
Copy link
Contributor

abailly-akamai commented Feb 16, 2024

@carrillo-erik it should be undefined, because this key does not exist in the upstream schema

Copy link
Contributor

@jaalah-akamai jaalah-akamai left a comment

Choose a reason for hiding this comment

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

Do we need a changeset for APIv4?

@jaalah-akamai jaalah-akamai changed the title Add placement group to payload upcoming: [M3-7615] - Add placement group to payload Feb 17, 2024
@jaalah-akamai
Copy link
Contributor

@carrillo-erik When the error appears the tooltip icon shifts down. We should remove the tooltip icon here and just pass textFieldProps to <PlacementGroupsSelect />:

textFieldProps={{tooltipText}}

Obviously when you call the underlying <Autocomplete /> component you'll need to spread the props there {...textFieldProps} and maybe even add a tooltipText prop to the <TextField /> component itself in Autocomplete.tsx.

@jaalah-akamai jaalah-akamai added the Add'tl Approval Needed Waiting on another approval! label Feb 17, 2024
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 the updates! Approving pending change suggested by @jaalah-akamai

@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Feb 20, 2024
@carrillo-erik carrillo-erik merged commit 65a84e9 into linode:develop Feb 20, 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! Placement Groups
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants