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-7779]: Placement Groups types update, part 2 #10200

Merged
merged 10 commits into from
Feb 27, 2024

Conversation

abailly-akamai
Copy link
Contributor

@abailly-akamai abailly-akamai commented Feb 16, 2024

Description 📝

This PR bring the latest and greatest API specs updates to the Placement Group feature.

The functionality should be identical and no UI regression is expected from those changes

Changes 🔄

  • The response from the GET PlacementGroup.linodes changes from an array of linode IDs to an array of linode objects to include compliance for each linode
  • RenamePlacementGroupPayload becomes UpdatePlacementGroupPayload
  • compliant_only becomes a silent parameter
  • methods and utils updates
  • factories and test updated

Note: Not adding a changeset for apiV4 is intentional

Preview 📷

There should be no UI change as a result of this PR

How to test 🧪

Prerequisites

  • Turn on both the "Placement Group" flag and MSW

Verification steps

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

@abailly-akamai abailly-akamai self-assigned this Feb 16, 2024
@abailly-akamai abailly-akamai changed the title upcoming: [M3-7770]: Update Linode and PlacementGroup types upcoming: [M3-7770]: Placement Groups types update, part 2 Feb 16, 2024
@abailly-akamai abailly-akamai changed the title upcoming: [M3-7770]: Placement Groups types update, part 2 upcoming: [M3-7779]: Placement Groups types update, part 2 Feb 22, 2024
@abailly-akamai abailly-akamai force-pushed the M3-7779 branch 2 times, most recently from 6460aac to 0107d5d Compare February 26, 2024 16:10
id?: number | null;
compliant_only?: boolean | null;
id: number;
compliant_only?: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a new undocumented parameter that should be represented in our types ( for API fidelity & SDK purpose) but won't be utilized in Cloud Manager

linode: number;
is_compliant: boolean;
}[];
is_strict: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are the core changes for this PR, resulting in the changes below.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's nice to finally have things in sync between specs and our codebase.

region: Region['id'];
};

export type UpdatePlacementGroupPayload = Pick<PlacementGroup, 'label'>;
Copy link
Contributor Author

@abailly-akamai abailly-akamai Feb 27, 2024

Choose a reason for hiding this comment

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

RenamePlacementGroupPayload isn't the best term from an API standpoint, changing to UpdatePlacementGroupPayload to reflect better the HTTP method

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

Changes look good pending checks pass. All feedback is optional

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

github-actions bot commented Feb 27, 2024

Coverage Report:
Base Coverage: 81.34%
Current Coverage: 81.34%

@abailly-akamai abailly-akamai merged commit f5f53de into linode:develop Feb 27, 2024
17 of 18 checks passed
@carrillo-erik
Copy link
Contributor

@abailly-akamai Nice work all around! Everything looks good and ready to go.

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