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.

Sign up for GitHub

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

Jump to bottom

upcoming: [M3-8420] – Add conditionally-displayed reboot notice on Volume Create page #10868

Merged
merged 9 commits into from
Sep 10, 2024
Merged

upcoming: [M3-8420] – Add conditionally-displayed reboot notice on Volume Create page #10868

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
dc4aae7
Conditionally display client update required copy in Volume Create fl…
DevDW Aug 30, 2024
bd5298c
Sort imports
DevDW Sep 3, 2024
70aa5a6
Align items to baseline, remove conditional
DevDW Sep 3, 2024
b53e86d
Only display client library update notice if checkbox has been checke…
DevDW Sep 4, 2024
641e94c
E2E coverage for scenarios on Volume Create page
DevDW Sep 4, 2024
55ee22d
Added changeset: Add conditional client library update required reboo…
DevDW Sep 5, 2024
98146d1
Tighten up disabled logic for Create Volume button
DevDW Sep 5, 2024
fa29499
Merge in latest develop; resolve conflicts
DevDW Sep 9, 2024
ac9fefa
E2E coverage for case where selected linode supports BSE
DevDW Sep 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/manager/.changeset/pr-10868-upcoming-features-1725546685590.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@linode/manager": Upcoming Features
---

Add conditional client library update required reboot notice to Volume Create page ([#10868](https://github.com/linode/manager/pull/10868))
57 changes: 57 additions & 0 deletions packages/manager/cypress/e2e/core/volumes/create-volume.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,10 @@ describe('volume create flow', () => {
.should('be.visible')
.click();

// @TODO BSE: once BSE is fully rolled out, check for the notice (selected linode doesn't have
// blockstorage_encryption capability + user checked "Encrypt Volume" checkbox) instead of the absence of it
cy.findByText(CLIENT_LIBRARY_UPDATE_COPY).should('not.exist');
Copy link
Contributor

Choose a reason for hiding this comment

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

When manually testing, rebooting a linode without the bse capability didn't result in an upgraded qemu version/bse capability for me - not sure if that's expected in dev. I might have missed it, but wasn't seeing anywhere in tests where we were confirming that a user with a bse capability on the linode would no longer see the checkbox. Did we want to confirm this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Raised the reboot --> upgrade question in the project channel, not sure if that is fully in place yet.

Added test coverage for the case where the linode supports BSE


cy.findByText('Create Volume').click();
cy.wait('@createVolume');

Expand Down Expand Up @@ -161,6 +165,59 @@ describe('volume create flow', () => {
);
});

/*
* - Checks for Block Storage Encryption notices on the Volume Create page.
*/
it('displays a warning notice on Volume Create page re: rebooting for client library updates under the appropriate conditions', () => {
// Conditions: Block Storage encryption feature flag is on; user has Block Storage Encryption capability; volume being created is encrypted and the
// selected Linode does not support Block Storage Encryption

// Mock feature flag -- @TODO BSE: Remove feature flag once BSE is fully rolled out
mockAppendFeatureFlags({
blockStorageEncryption: true,
}).as('getFeatureFlags');

// Mock account response
const mockAccount = accountFactory.build({
capabilities: ['Linodes', 'Block Storage Encryption'],
});

mockGetAccount(mockAccount).as('getAccount');
mockGetRegions(mockRegions).as('getRegions');

const linodeRequest = createLinodeRequestFactory.build({
label: randomLabel(),
root_pass: randomString(16),
region: mockRegions[0].id,
booted: false,
});

cy.defer(() => createTestLinode(linodeRequest), 'creating Linode').then(
(linode: Linode) => {
cy.visitWithLogin('/volumes/create');
cy.wait(['@getFeatureFlags', '@getAccount']);

// Select a linode without the BSE capability
cy.findByLabelText('Linode')
.should('be.visible')
.click()
.type(linode.label);

ui.autocompletePopper
.findByTitle(linode.label)
.should('be.visible')
.click();

// Check the "Encrypt Volume" checkbox
cy.get('[data-qa-checked]').should('be.visible').click();
// });

// Ensure warning notice is displayed
cy.findByText(CLIENT_LIBRARY_UPDATE_COPY).should('be.visible');
}
);
});

/*
* - Checks for Block Storage Encryption notices in the Create/Attach Volume drawer from the
'Storage' details page of an existing Linode.
Expand Down
110 changes: 71 additions & 39 deletions packages/manager/src/features/Volumes/VolumeCreate.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { Button } from 'src/components/Button/Button';
import { DocumentTitleSegment } from 'src/components/DocumentTitle';
import {
BLOCK_STORAGE_CHOOSE_REGION_COPY,
BLOCK_STORAGE_CLIENT_LIBRARY_UPDATE_REQUIRED_COPY,
BLOCK_STORAGE_ENCRYPTION_GENERAL_DESCRIPTION,
BLOCK_STORAGE_ENCRYPTION_OVERHEAD_CAVEAT,
BLOCK_STORAGE_ENCRYPTION_UNAVAILABLE_IN_REGION_COPY,
Expand All @@ -23,6 +24,7 @@ import { LandingHeader } from 'src/components/LandingHeader';
import { Notice } from 'src/components/Notice/Notice';
import { Paper } from 'src/components/Paper';
import { RegionSelect } from 'src/components/RegionSelect/RegionSelect';
import { Stack } from 'src/components/Stack';
import { TextField } from 'src/components/TextField';
import { TooltipIcon } from 'src/components/TooltipIcon';
import { Typography } from 'src/components/Typography';
Expand All @@ -35,6 +37,7 @@ import {
useAccountAgreements,
useMutateAccountAgreements,
} from 'src/queries/account/agreements';
import { useLinodeQuery } from 'src/queries/linodes/linodes';
import { useGrants, useProfile } from 'src/queries/profile/profile';
import { useRegionsQuery } from 'src/queries/regions/regions';
import {
Expand Down Expand Up @@ -240,6 +243,15 @@ export const VolumeCreate = () => {

const { config_id, linode_id } = values;

const { data: linode } = useLinodeQuery(
linode_id ?? -1,
isBlockStorageEncryptionFeatureEnabled
Copy link
Contributor

Choose a reason for hiding this comment

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

Late to this PR, but noticed a minor issue that can be resolved in a later change:

To prevent an API call for linodes/instances/-1

Suggested change
isBlockStorageEncryptionFeatureEnabled
isBlockStorageEncryptionFeatureEnabled && linode_id !== null

);

const linodeSupportsBlockStorageEncryption = Boolean(
linode?.capabilities?.includes('blockstorage_encryption')
);

const linodeError = touched.linode_id ? errors.linode_id : undefined;

const { showGDPRCheckbox } = getGDPRDetails({
Expand Down Expand Up @@ -283,6 +295,11 @@ export const VolumeCreate = () => {
}
};

const shouldDisplayClientLibraryCopy =
isBlockStorageEncryptionFeatureEnabled &&
values.linode_id !== null &&
!linodeSupportsBlockStorageEncryption;

return (
<>
<DocumentTitleSegment segment="Create a Volume" />
Expand Down Expand Up @@ -363,47 +380,57 @@ export const VolumeCreate = () => {
)}
</Box>
<Box
alignItems="flex-end"
alignItems="baseline"
className={classes.linodeConfigSelectWrapper}
display="flex"
>
<Box
alignItems="flex-end"
className={classes.linodeSelect}
display="flex"
>
<LinodeSelect
optionsFilter={(linode: Linode) => {
const linodeRegion = linode.region;
const valuesRegion = values.region;

/** When values.region is empty, all Linodes with
* block storage support will be displayed, regardless
* of their region. However, if a region is selected,
* only Linodes from the chosen region with block storage
* support will be shown. */
return isNilOrEmpty(valuesRegion)
? regionsWithBlockStorage.includes(linodeRegion)
: regionsWithBlockStorage.includes(linodeRegion) &&
linodeRegion === valuesRegion;
}}
sx={{
[theme.breakpoints.down('sm')]: {
width: 320,
},
width: '400px',
}}
clearable
disabled={doesNotHavePermission}
errorText={linodeError}
onBlur={handleBlur}
onSelectionChange={handleLinodeChange}
value={values.linode_id}
/>
{renderSelectTooltip(
'If you select a Linode, the Volume will be automatically created in that Linode’s region and attached upon creation.'
)}
</Box>
<Stack>
<Box
alignItems="flex-end"
className={classes.linodeSelect}
display="flex"
>
<LinodeSelect
optionsFilter={(linode: Linode) => {
const linodeRegion = linode.region;
const valuesRegion = values.region;

/** When values.region is empty, all Linodes with
* block storage support will be displayed, regardless
* of their region. However, if a region is selected,
* only Linodes from the chosen region with block storage
* support will be shown. */
return isNilOrEmpty(valuesRegion)
? regionsWithBlockStorage.includes(linodeRegion)
: regionsWithBlockStorage.includes(linodeRegion) &&
linodeRegion === valuesRegion;
}}
sx={{
[theme.breakpoints.down('sm')]: {
width: 320,
},
width: '400px',
}}
clearable
disabled={doesNotHavePermission}
errorText={linodeError}
onBlur={handleBlur}
onSelectionChange={handleLinodeChange}
value={values.linode_id}
/>
{renderSelectTooltip(
'If you select a Linode, the Volume will be automatically created in that Linode’s region and attached upon creation.'
)}
</Box>
{shouldDisplayClientLibraryCopy &&
values.encryption === 'enabled' && (
<Notice spacingBottom={0} spacingTop={16} variant="warning">
<Typography maxWidth="416px">
{BLOCK_STORAGE_CLIENT_LIBRARY_UPDATE_REQUIRED_COPY}
</Typography>
</Notice>
)}
</Stack>
<ConfigSelect
disabled={doesNotHavePermission || config_id === null}
error={touched.config_id ? errors.config_id : undefined}
Expand Down Expand Up @@ -473,6 +500,12 @@ export const VolumeCreate = () => {
</Paper>
<Box display="flex" justifyContent="flex-end">
<Button
disabled={
disabled ||
(isBlockStorageEncryptionFeatureEnabled &&
!linodeSupportsBlockStorageEncryption &&
values.encryption === 'enabled')
}
tooltipText={
!isLoading && isInvalidPrice
? PRICES_RELOAD_ERROR_NOTICE_TEXT
Expand All @@ -481,7 +514,6 @@ export const VolumeCreate = () => {
buttonType="primary"
className={classes.button}
data-qa-deploy-linode
disabled={disabled}
loading={isSubmitting}
style={{ marginLeft: 12 }}
type="submit"
Expand Down
Loading