feat: [M3-8985] - High performance volume indicator #11400

dchyrva-akamai · 2024-12-11T13:11:19Z

Description 📝

Added high performance volume indicator to the Linode details panel and Linode volumes table.

Changes 🔄

Created "HighPerformanceVolumeIcon" component.
The icon component used to indicate high performance volume on the Linode summary panel.
The icon component used to indicate high performance volume in the Linode Volumes table.

Preview 📷

How to test 🧪

Create a Linode with 32GB of RAM.
Attach any Volume to the newly created Linode.
Observe whether the icon appeared or not.

As an Author, to speed up the review process, I considered 🤔

👀 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

I have read and considered all applicable items listed above.

As an Author, before moving this PR from Draft to Open, I confirmed ✅

All unit tests are passing
TypeScript compilation succeeded without errors
Code passes all linting rules

github-actions · 2024-12-11T13:46:50Z

Coverage Report: ✅
Base Coverage: 86.98%
Current Coverage: 86.98%

mjac0bs

Thanks for the contribution @dchyrva-akamai!

I'm missing some context on this feature. How do we get the necessary new Linode capability? Is this capability currently being returned on any production Linode with 32GB (or more?) RAM? The screenshots show a 1GB Linode. Are there any customer tags necessary for the new capability?

Following the testing steps, I created a 32GB Linode, attached a volume to that Linode, and was not seeing the new Block Storage Performance B1 returned from the linode's prod API endpoint, so therefore saw no high perf icon. (The same was true in the dev environment.)

Note to fellow reviews: On my prod account, I had to first increase my Reputation in Admin to 100 to be able to create a linode with that plan and avoid encountering an account limit error.

packages/manager/.changeset/pr-11400-added-1733927873131.md

packages/api-v4/src/linodes/types.ts

bnussman-akamai

Thanks for opening this PR! Looks great, I just have one stylistic suggestion

packages/manager/src/features/Volumes/VolumeTableRow.tsx

dchyrva-akamai · 2024-12-12T14:18:52Z

Thanks for the contribution @dchyrva-akamai!

I'm missing some context on this feature. How do we get the necessary new Linode capability? Is this capability currently being returned on any production Linode with 32GB (or more?) RAM? The screenshots show a 1GB Linode. Are there any customer tags necessary for the new capability?

Following the testing steps, I created a 32GB Linode, attached a volume to that Linode, and was not seeing the new Block Storage Performance B1 returned from the linode's prod API endpoint, so therefore saw no high perf icon. (The same was true in the dev environment.)

Note to fellow reviews: On my prod account, I had to first increase my Reputation in Admin to 100 to be able to create a linode with that plan and avoid encountering an account limit error.

According to the information available to me in the ticket, availability of the Block Storage Performance B1 capability should activate the icon.

There is a chance that the Back-End side of the feature has not been implemented yet, which is why "How to test" section might be misleading at the moment. The screenshots in the "Preview" section were taken from the local environment.
Unfortunately, I am not in contact with the Back-End team responsible for this part of the feature and cannot provide you with the status of the implementation from their side.

skulpok-akamai · 2024-12-12T14:35:39Z

Thanks for the contribution @dchyrva-akamai!
I'm missing some context on this feature. How do we get the necessary new Linode capability? Is this capability currently being returned on any production Linode with 32GB (or more?) RAM? The screenshots show a 1GB Linode. Are there any customer tags necessary for the new capability?
Following the testing steps, I created a 32GB Linode, attached a volume to that Linode, and was not seeing the new Block Storage Performance B1 returned from the linode's prod API endpoint, so therefore saw no high perf icon. (The same was true in the dev environment.)

Note to fellow reviews: On my prod account, I had to first increase my Reputation in Admin to 100 to be able to create a linode with that plan and avoid encountering an account limit error.

According to the information available to me in the ticket, availability of the Block Storage Performance B1 capability should activate the icon.

There is a chance that the Back-End side of the feature has not been implemented yet, which is why "How to test" section might be misleading at the moment. The screenshots in the "Preview" section were taken from the local environment. Unfortunately, I am not in contact with the Back-End team responsible for this part of the feature and cannot provide you with the status of the implementation from their side.

Hi mjac0bs, most likely the feature has not been rolled out yet on the API side (I'm trying to find out if that's the case). We decided to create a PR for this feature to get early feedback, and also because these are non-breaking changes - there are no visual changes in the UI when Block Storage Performance B1 is not present, so it's not crucial for the API part to be implemented first. I understand that it's hard to test this feature in this state (an instance object must be altered from the browser console to include Block Storage Performance B1), so we can wait with merging the changes into develop until we know that the API part is ready.

bnussman-akamai · 2024-12-12T15:16:00Z

packages/manager/src/features/Linodes/HighPerformanceVolumeIcon.tsx

+  return (
+    isHighPerformanceVolume && (
+      <Tooltip arrow title="High Performance">
+        <IconButton
+          sx={{
+            border: '1px solid',
+            borderRadius: '50%',
+            padding: 0,
+          }}
+        >
+          <BoltIcon sx={{ fontSize: 12 }} />
+        </IconButton>
+      </Tooltip>
+    )
+  );


Suggested change

return (

isHighPerformanceVolume && (

<Tooltip arrow title="High Performance">

<IconButton

sx={{

border: '1px solid',

borderRadius: '50%',

padding: 0,

}}

>

<BoltIcon sx={{ fontSize: 12 }} />

</IconButton>

</Tooltip>

)

);

if (!isHighPerformanceVolume) {

return null;

}

return (

<Tooltip arrow title="High Performance">

<IconButton

sx={{

border: '1px solid',

borderRadius: '50%',

padding: 0,

}}

>

<BoltIcon sx={{ fontSize: 12 }} />

</IconButton>

</Tooltip>

);

bnussman-akamai · 2024-12-12T15:16:27Z

packages/manager/src/features/Linodes/HighPerformanceVolumeIcon.tsx

+  );
+}
+
+export default HighPerformanceVolumeIcon;


Let's remove this default export and export the function instead. We prefer named exports in our codebase. (docs)

@bnussman-akamai Should also be resolved.

bnussman-akamai · 2024-12-12T15:16:38Z

packages/manager/src/features/Linodes/HighPerformanceVolumeIcon.tsx

+  linodeCapabilities?: LinodeCapabilities[];
+}
+
+function HighPerformanceVolumeIcon({ linodeCapabilities }: Props) {


Suggested change

function HighPerformanceVolumeIcon({ linodeCapabilities }: Props) {

export function HighPerformanceVolumeIcon({ linodeCapabilities }: Props) {

packages/manager/src/features/Volumes/VolumeTableRow.tsx

mjac0bs

Left a couple comments on spacing and below. Approving pending feedback is addressed; Banks had some good thoughts as well.

We decided to create a PR for this feature to get early feedback, and also because these are non-breaking changes

++ We appreciate this - it's helpful to be able to provide early feedback!

I understand that it's hard to test this feature in this state (an instance object must be altered from the browser console to include Block Storage Performance B1), so we can wait with merging the changes into develop until we know that the API part is ready.

We often mock API data and use our Mock Service Worker dev tool when doing development; I wanted to be sure that was what we'd need to do to test. We can modify code like the linodeSeeder to mock a linode with the new capability.

      seedEntities: linodeFactory.buildList(count, {
        capabilities: ['Block Storage Performance B1'],
      }),

Screen.Recording.2024-12-12.at.9.27.57.AM.mov

I recommend adding test coverage. For example, adding on to our existing VolumeTableRow.test.tsx:

describe('Volume table row - for linodes detail page', () => {
  it('should show a high performance icon tooltip if Linode has the capability', async () => {
    const { getByLabelText, getByText } = await renderWithThemeAndRouter(
      wrapWithTableBody(
        <VolumeTableRow
          handlers={handlers}
          isDetailsPageRow
          linodeCapabilities={['Block Storage Performance B1']}
          volume={attachedVolume}
        />
      )
    );

    const highPerformanceIcon = getByLabelText('High Performance');

    expect(highPerformanceIcon).toBeVisible();
    await userEvent.click(highPerformanceIcon);
    await waitFor(() => expect(getByText('High Performance')).toBeVisible());
  });

packages/manager/src/features/Linodes/LinodeEntityDetailBody.tsx

packages/manager/src/features/Volumes/VolumeTableRow.tsx

bnussman-akamai · 2024-12-13T16:23:51Z

packages/api-v4/src/linodes/types.ts

@@ -20,7 +20,7 @@ export interface Linode {
  id: number;
  alerts: LinodeAlerts;
  backups: LinodeBackups;
-  capabilities?: LinodeCapabilities[]; // @TODO BSE: Remove optionality once BSE is fully rolled out


I went ahead and pushed f80e9e9 to improve some types. I also removed the optionality here.

@dwiley-akamai @hkhalil-akamai Do you guys think this change is okay? From what I can see, capabilities is fully rolled out

Yes, we should be good to go with that. I'll circle back to review other instances as part of M3-8405

packages/api-v4/.changeset/pr-11400-added-1733998451458.md

dwiley-akamai · 2024-12-13T19:28:43Z

packages/api-v4/src/linodes/types.ts

@@ -20,7 +20,7 @@ export interface Linode {
  id: number;
  alerts: LinodeAlerts;
  backups: LinodeBackups;
-  capabilities?: LinodeCapabilities[]; // @TODO BSE: Remove optionality once BSE is fully rolled out


Yes, we should be good to go with that. I'll circle back to review other instances as part of M3-8405

packages/manager/.changeset/pr-11400-added-1733927873131.md

mjac0bs

Just noticed a couple more things.

(And pushed updates for them in cd87815. cc @dchyrva-akamai )

packages/manager/src/features/Linodes/HighPerformanceVolumeIcon.tsx

linode-gh-bot · 2024-12-16T10:06:10Z

Cloud Manager UI test results

🔺 1 failing test on test run #8 ↗︎

❌ Failing	✅ Passing	↪️ Skipped	🕐 Duration
`1 Failing`	`465 Passing`	`5 Skipped`	`103m 43s`

Details

Failing Tests
	Spec	Test
❌	`smoke-delete-linode.spec.ts`	delete linode » "before all" hook for "deletes linode from linode details page"

Troubleshooting

Use this command to re-run the failing tests:

yarn cy:run -s "cypress/e2e/core/linodes/smoke-delete-linode.spec.ts"

skulpok-akamai · 2024-12-16T15:04:55Z

We often mock API data and use our Mock Service Worker dev tool when doing development; I wanted to be sure that was what we'd need to do to test. We can modify code like the linodeSeeder to mock a linode with the new capability.

That's very useful, thanks for sharing!

mjac0bs · 2024-12-16T15:32:54Z

Feedback has been addressed and the last CI run had a test failure that is unrelated to this PR - looks like flake. I'm going to merge this now. Thanks @dchyrva-akamai and @skulpok-akamai 🚀

cypress · 2024-12-16T16:04:20Z

Cloud Manager E2E Run #6975

Run Properties: Failed #6975 • b703190465: feat: [M3-8985] - High performance volume indicator (#11400)

Project	`Cloud Manager E2E`
Branch Review	`develop`
Run status	`Failed #6975`
Run duration	`29m 41s`
Commit	`b703190465: feat: [M3-8985] - High performance volume indicator (#11400)`
Committer	`Dmytro Chyrva`
View all properties for this run ↗︎

Test results
Failures	`1`
Flaky	`0`
Pending	`2`
Skipped	`0`
Passing	`465`
View all changes introduced in this branch ↗︎

Tests for review

cypress/e2e/core/linodes/clone-linode.spec.ts • 1 failed test

View Output Video

Test		Artifacts
clone linode > can clone a Linode from Linode details page		`Screenshots` `Video`

feat: [M3-8985] - High performance volume indicator added.

5b98ec2

dchyrva-akamai requested a review from a team as a code owner December 11, 2024 13:11

dchyrva-akamai requested review from mjac0bs and carrillo-erik and removed request for a team December 11, 2024 13:11

dchyrva-akamai marked this pull request as draft December 11, 2024 13:13

skulpok-akamai approved these changes Dec 11, 2024

View reviewed changes

dchyrva-akamai marked this pull request as ready for review December 11, 2024 15:39

mjac0bs reviewed Dec 11, 2024

View reviewed changes

packages/manager/.changeset/pr-11400-added-1733927873131.md Outdated Show resolved Hide resolved

packages/api-v4/src/linodes/types.ts Show resolved Hide resolved

mjac0bs assigned dchyrva-akamai Dec 11, 2024

dchyrva-akamai added 2 commits December 12, 2024 11:12

Added changeset: High performance volume indicator added.

817a43a

Added changeset: New Block Storage Performance B1 linode capability

bf72d0a

dchyrva-akamai force-pushed the feature/M3-8985 branch from 2d66906 to bf72d0a Compare December 12, 2024 10:14

bnussman-akamai reviewed Dec 12, 2024

View reviewed changes

packages/manager/src/features/Volumes/VolumeTableRow.tsx Outdated Show resolved Hide resolved

bnussman-akamai reviewed Dec 12, 2024

View reviewed changes

mjac0bs approved these changes Dec 12, 2024

View reviewed changes

packages/manager/src/features/Linodes/LinodeEntityDetailBody.tsx Outdated Show resolved Hide resolved

packages/manager/src/features/Linodes/LinodeEntityDetailBody.tsx Outdated Show resolved Hide resolved

packages/manager/src/features/Volumes/VolumeTableRow.tsx Outdated Show resolved Hide resolved

mjac0bs added the Add'tl Approval Needed Waiting on another approval! label Dec 12, 2024

bnussman-akamai changed the title ~~feat: [M3-8985] - High performance volume indicator added.~~ feat: [M3-8985] - High performance volume indicator Dec 12, 2024

bnussman-akamai added the Volumes Relating to Volumes (aka Block Storage) label Dec 12, 2024

feat: [M3-8985] - Comments resolved.

4f7c53c

dchyrva-akamai force-pushed the feature/M3-8985 branch from 014ef0f to 4f7c53c Compare December 13, 2024 09:21

improve prop optionality

f80e9e9

bnussman-akamai reviewed Dec 13, 2024

View reviewed changes

dwiley-akamai self-requested a review December 13, 2024 17:07

dwiley-akamai reviewed Dec 13, 2024

View reviewed changes

mjac0bs reviewed Dec 13, 2024

View reviewed changes

packages/manager/src/features/Linodes/HighPerformanceVolumeIcon.tsx Outdated Show resolved Hide resolved

packages/manager/src/features/Linodes/HighPerformanceVolumeIcon.tsx Outdated Show resolved Hide resolved

Use ui package components and keep tooltip styling consistent

cd87815

mjac0bs and others added 2 commits December 13, 2024 12:17

Update changesets for Upcoming Features

5866a1b

Small fix.

46a680a

bnussman-akamai approved these changes Dec 16, 2024

View reviewed changes

bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Dec 16, 2024

mjac0bs merged commit b703190 into linode:develop Dec 16, 2024
22 of 23 checks passed

dchyrva-akamai deleted the feature/M3-8985 branch December 17, 2024 08:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [M3-8985] - High performance volume indicator #11400

feat: [M3-8985] - High performance volume indicator #11400

dchyrva-akamai commented Dec 11, 2024 •

edited

Loading

github-actions bot commented Dec 11, 2024 •

edited

Loading

mjac0bs left a comment

bnussman-akamai left a comment

dchyrva-akamai commented Dec 12, 2024

skulpok-akamai commented Dec 12, 2024 •

edited

Loading

bnussman-akamai Dec 12, 2024

bnussman-akamai Dec 12, 2024

dchyrva-akamai Dec 13, 2024

bnussman-akamai Dec 12, 2024

mjac0bs left a comment •

edited

Loading

bnussman-akamai Dec 13, 2024 •

edited

Loading

dwiley-akamai Dec 13, 2024

dwiley-akamai Dec 13, 2024

mjac0bs left a comment •

edited

Loading

linode-gh-bot commented Dec 16, 2024

skulpok-akamai commented Dec 16, 2024

mjac0bs commented Dec 16, 2024

cypress bot commented Dec 16, 2024

	function HighPerformanceVolumeIcon({ linodeCapabilities }: Props) {
	export function HighPerformanceVolumeIcon({ linodeCapabilities }: Props) {

feat: [M3-8985] - High performance volume indicator #11400

feat: [M3-8985] - High performance volume indicator #11400

Conversation

dchyrva-akamai commented Dec 11, 2024 • edited Loading

Description 📝

Changes 🔄

Preview 📷

How to test 🧪

As an Author, to speed up the review process, I considered 🤔

As an Author, before moving this PR from Draft to Open, I confirmed ✅

github-actions bot commented Dec 11, 2024 • edited Loading

mjac0bs left a comment

Choose a reason for hiding this comment

bnussman-akamai left a comment

Choose a reason for hiding this comment

dchyrva-akamai commented Dec 12, 2024

skulpok-akamai commented Dec 12, 2024 • edited Loading

bnussman-akamai Dec 12, 2024

Choose a reason for hiding this comment

bnussman-akamai Dec 12, 2024

Choose a reason for hiding this comment

dchyrva-akamai Dec 13, 2024

Choose a reason for hiding this comment

bnussman-akamai Dec 12, 2024

Choose a reason for hiding this comment

mjac0bs left a comment • edited Loading

Choose a reason for hiding this comment

bnussman-akamai Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

dwiley-akamai Dec 13, 2024

Choose a reason for hiding this comment

dwiley-akamai Dec 13, 2024

Choose a reason for hiding this comment

mjac0bs left a comment • edited Loading

Choose a reason for hiding this comment

linode-gh-bot commented Dec 16, 2024

Cloud Manager UI test results

🔺 1 failing test on test run #8 ↗︎

Details

Troubleshooting

skulpok-akamai commented Dec 16, 2024

mjac0bs commented Dec 16, 2024

cypress bot commented Dec 16, 2024

Cloud Manager E2E Run #6975

Tests for review

dchyrva-akamai commented Dec 11, 2024 •

edited

Loading

github-actions bot commented Dec 11, 2024 •

edited

Loading

skulpok-akamai commented Dec 12, 2024 •

edited

Loading

mjac0bs left a comment •

edited

Loading

bnussman-akamai Dec 13, 2024 •

edited

Loading

mjac0bs left a comment •

edited

Loading