Skip to content

refactor: [M3-8496] - Remove Placement Group flag and underlying logic#10877

Merged
abailly-akamai merged 3 commits intolinode:developfrom
abailly-akamai:M3-8496
Sep 6, 2024
Merged

refactor: [M3-8496] - Remove Placement Group flag and underlying logic#10877
abailly-akamai merged 3 commits intolinode:developfrom
abailly-akamai:M3-8496

Conversation

@abailly-akamai
Copy link
Contributor

@abailly-akamai abailly-akamai commented Sep 3, 2024

Description 📝

Update CM codebase to remove placementGroups flag logic.

This is should bring no changes to the feature nor introduce any regressions

Changes 🔄

List any change relevant to the reviewer.

  • Remove flag
  • Remove flag tool
  • Update unit & e2e tests

How to test 🧪

Prerequisites

Verification steps

  • Verify Placement Groups feature is available as a general feature (main navigation and feature details)
  • Confirm tests are accurate and passing without flag logic

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 Sep 3, 2024
@abailly-akamai abailly-akamai marked this pull request as ready for review September 3, 2024 21:00
@abailly-akamai abailly-akamai requested review from a team as code owners September 3, 2024 21:00
@abailly-akamai abailly-akamai requested review from carrillo-erik, jdamore-linode and mjac0bs and removed request for a team September 3, 2024 21:00
@github-actions
Copy link

github-actions bot commented Sep 3, 2024

Coverage Report:
Base Coverage: 86.2%
Current Coverage: 86.2%

Copy link
Contributor

@carrillo-erik carrillo-erik 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 taking care of this and preventing stale code from remaining in the project. ✅

Copy link
Contributor

@carrillo-erik carrillo-erik left a comment

Choose a reason for hiding this comment

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

These are the culprits causing the failures in the PR. Removing these should do the trick.

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.

(Found a couple more comments to clean up in the tests, as well.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Mock the VM Placement Groups feature flag to be enabled for each test in this block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

Comment on lines 49 to 50
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Mock the VM Placement Groups feature flag to be enabled for each test in this block.
// TODO Remove these mocks when `placementGroups` feature flag is retired.

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, this should be set now! 🚢

@mjac0bs mjac0bs added the Add'tl Approval Needed Waiting on another approval! label Sep 5, 2024
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.

🧹

@jaalah-akamai jaalah-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Sep 5, 2024
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

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants