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-8012] - Fix PG event text formatting #10420

Merged
merged 8 commits into from
May 2, 2024

Conversation

carrillo-erik
Copy link
Contributor

@carrillo-erik carrillo-erik commented Apr 30, 2024

Description 📝

This PR updates the types and constants for action events for Placement Groups. Specifically, the action events are updated with a present-tense and imperative-style for consistency with other entity events. These changes resolve issues with Placement Group events missing the link and username.

Changes 🔄

List any change relevant to the reviewer.

  • Remove the d and ed from events to make them consistent with other entity action events.
    (Ex. placement_group_created becomes placement_group_create and placement_group_unassigned becomes placement_group_unassign.

Preview 📷

Before After
before-aaa after-bb

How to test 🧪

Prerequisites

(How to setup test environment)

  • Have the placement-group customer tag on the user account used for testing.
  • Using the Cloud Manager Dev Tools:
    • Enable the Placement Groups feature flag
    • Switch to Dev environment
    • Disable the MSW

Verification steps

(How to verify changes)

  • Verify the following events emit an event that is formatted properly (with link and username).
    • placement_group_create
    • placement_group_assign
    • placement_group_unassign
    • placement_group_update
    • placement_group_delete

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 self-assigned this Apr 30, 2024
@carrillo-erik carrillo-erik requested a review from a team as a code owner April 30, 2024 13:28
@carrillo-erik carrillo-erik requested review from mjac0bs, bnussman-akamai and abailly-akamai and removed request for a team April 30, 2024 13:28
Copy link

github-actions bot commented Apr 30, 2024

Coverage Report:
Base Coverage: 81.82%
Current Coverage: 81.82%

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.

✅ Confirmed that the PG event verb tenses are consistent with our existing events.
✅ Confirmed that user name, the correct action for create/update/delete/unassign, and correct linkable entities show up in the events lists.

A couple of questions about consistency with toasts, which are slightly tangential to this event PR, so I'll approve once there's clarity with the assign/assigned events:

  • Can we punctuate consistently in toasts? This has been a cafe discussion before, so I'm mentioning here. I think they can be considered a sentence (subject and verb, even if it is a fragment) with a period.

Has period (delete only):
Screenshot 2024-04-30 at 8 14 26 AM

Does not have period (everything else):
Screenshot 2024-04-30 at 8 14 58 AM
Screenshot 2024-04-30 at 8 14 36 AM
Screenshot 2024-04-30 at 8 16 25 AM
Screenshot 2024-04-30 at 8 15 16 AM

  • Is it an intentional choice (for brevity, maybe) to not mention what the linode has been assigned/unassigned from in the toasts? (e.g. 'Linode successfully unassigned from Placement Group.') This differs from the event message, which does mention a PG.
    Screenshot 2024-04-30 at 8 16 57 AM

@carrillo-erik
Copy link
Contributor Author

After removing the placement_group_assigned event, the events were manually tested. The image below depicts a workflow starting from the Create Linode form.

  1. A Placement Group is created
  2. The Linode is created and is assigned to the newly create Placement Group.
  3. A new stand alone Linode instance is created in the same region as the Placement Group from step 2.
  4. From the Placement Group details page, the Linode instance from step 3 is assigned via thePlacementGroupAssignLinodeDrawer.
  5. The event message formatting (entity label, username, and entity links) is verified in the Events page.

Screenshot 2024-05-01 at 7 02 18 AM

@carrillo-erik
Copy link
Contributor Author

  • Can we punctuate consistently in toasts? This has been a cafe discussion before, so I'm mentioning here. I think they can be considered a sentence (subject and verb, even if it is a fragment) with a period.
  • Is it an intentional choice (for brevity, maybe) to not mention what the linode has been assigned/unassigned from in the toasts? (e.g. 'Linode successfully unassigned from Placement Group.') This differs from the event message, which does mention a PG.

@mjac0bs I have updated the toast notifications to include the entity label and punctuation as suggested.

Screenshot 2024-05-01 at 8 48 18 AM
Screenshot 2024-05-01 at 8 46 01 AM
Screenshot 2024-05-01 at 8 45 52 AM

carrillo-erik and others added 2 commits May 1, 2024 08:55
…72551.md

Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
…130384.md

Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
@carrillo-erik carrillo-erik requested a review from mjac0bs May 1, 2024 15:58
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 for the updates -- confirmed that the event messages and their corresponding toasts are looking consistent. How does a placement group become compliant or non-compliant?

The CI e2e failures are unrelated, and I believe they could be fixed by merging in develop.

@carrillo-erik
Copy link
Contributor Author

How does a placement group become compliant or non-compliant?

From the API docs:
Screenshot 2024-05-01 at 2 07 18 PM

My understanding is that we don't trigger these events and just listen to them. We have logic that prevents the assignment of Linodes that would break compliance and the user is shown a toast notification of the failure. There's actions that take place in the backend that are async, which is where these might get triggered. Maybe @abailly-akamai can chime in and correct me where I'm wrong.

@abailly-akamai
Copy link
Contributor

abailly-akamai commented May 1, 2024

@carrillo-erik @mjac0bs sounds right to me. We just really want to populate those events. A placement group can fall out of compliance for reasons out of our control (usually due to availability, hardware failure etc). Completely async. On top of that event, the user will see a non-compliant status for the PG in Cloud Manager.

@carrillo-erik carrillo-erik merged commit 96a4051 into linode:develop May 2, 2024
17 of 18 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.

3 participants