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

change: [M3-8327] - Placement Groups General Availability API/UI updates #10651

Merged
merged 10 commits into from
Jul 16, 2024

Conversation

abailly-akamai
Copy link
Contributor

@abailly-akamai abailly-akamai commented Jul 8, 2024

Description 📝

Warning

This PR brings breaking changes to Cloud Manager, awaiting a matching APIv4 update to be released the same day

In preparation for Placement Group General Availability, the product team has identified needed breaking changes to APIv4 and Cloud Manager to help our users understand and use the feature more efficiently. As a result, the following changes have been implemented in preparation of a coordinated release on 7/22

Changes 🔄

APIv4:

  • replace affinity_type with placement_group_type
  • replace is_strict with placement_group_policy - enum('strict', 'flexible')

Cloud Manager:

  • Replace "Affinity Type" with "Placement Group Type"
  • Replace "Affinity Type Enforcement" with "Placement Group Policy"
  • Empty State Landing Page: "Control the physical placement of compute instances within a compute region"
  • Related changes
    • Util name changes
    • Interface name changes
    • Component name change
    • Update unit tests
    • Update e2e test

Target release date 🗓️

Caution

7/22/2024

Preview 📷

Before After
Placement Groups 1 (1) Placement Groups
Placement Groups 1 Placement Groups (2)
Placement Groups (4) Placement Groups (1)

How to test 🧪

Prerequisites

This PR will break current functionality (create PG) and display missing data in production.
As a result, it must be tested either in alpha or with MSW turned on.

Verification steps

  • Confirm nomenclature & copy changes

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

size: 6,
chart_type: 'area',
unit: 'OPS',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

those are just linting changes

@abailly-akamai abailly-akamai marked this pull request as ready for review July 9, 2024 16:53
@abailly-akamai abailly-akamai requested review from a team as code owners July 9, 2024 16:53
@abailly-akamai abailly-akamai requested review from jdamore-linode, carrillo-erik and AzureLatte and removed request for a team July 9, 2024 16:53
"@linode/api-v4": Changed
---

Breaking: change Placement Group `is_strict` to `placement_group_policy` ([#10651](https://github.com/linode/manager/pull/10651))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@johnwcallahan please let me know your thoughts about those. They are a pretty important part of communicating breaking changes to our customers. To that effect I added an APIv4 changeset for each change since both are breaking. Open to suggestions on the content.

Copy link

github-actions bot commented Jul 9, 2024

Coverage Report:
Base Coverage: 82.4%
Current Coverage: 82.4%

export const PlacementGroupsAffinityTypeEnforcementRadioGroup = (
props: Props
) => {
const ariaIdentifier = 'placement-group-policy-radio-group';
Copy link
Contributor

Choose a reason for hiding this comment

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

Small nitpick: consider moving this const to the constants.ts in the same directory.

@carrillo-erik
Copy link
Contributor

I've reviewed the changes, a good portion of them are just changing value names or strings to reflect the new copy. I tested the common workflows in alpha and did not encounter any unexpected behaviors. I would like to review this a second time, I'm approving for now since unit tests and ci checks passed and I didn't find any glaring issues.

@abailly-akamai
Copy link
Contributor Author

@carrillo-erik @jdamore-linode @AzureLatte can i get reviews/approvals for this one? been open for a week now, and it's a crucial PR to go along a breaking production API change so it needs to make the cut for the 7/17 release branch

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.

Didn't find any issues when testing against dev ✅

@abailly-akamai abailly-akamai merged commit 88296e7 into linode:develop Jul 16, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants