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-7927] - Linode Create Refactor - Part 6 - Add-ons #10319

Merged
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@linode/api-v4": Changed
---

Added jsdoc style comments to `CreateLinodeRequest` based on API documentation ([#10319](https://github.com/linode/manager/pull/10319))
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@linode/api-v4": Changed
---

Allows `firewall_id` to be `null` in `CreateLinodeRequest` ([#10319](https://github.com/linode/manager/pull/10319))
82 changes: 81 additions & 1 deletion packages/api-v4/src/linodes/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -350,24 +350,104 @@ export interface CreateLinodePlacementGroupPayload {
}

export interface CreateLinodeRequest {
/**
* The Linode Type of the Linode you are creating.
*/
type: string;
/**
* The Region where the Linode will be located.
*/
region: string;
/**
* A StackScript ID that will cause the referenced StackScript to be run during deployment of this Linode.
*
* This field cannot be used when deploying from a Backup or a Private Image.
*/
stackscript_id?: number;
/**
* A Backup ID from another Linode’s available backups.
*
* Your User must have read_write access to that Linode,
* the Backup must have a status of successful,
* and the Linode must be deployed to the same region as the Backup.
*
* This field and the image field are mutually exclusive.
*/
backup_id?: number;
/**
* When deploying from an Image, this field is optional, otherwise it is ignored.
* This is used to set the swap disk size for the newly-created Linode.
* @default 512
*/
swap_size?: number;
/**
* An Image ID to deploy the Linode Disk from.
*/
image?: string | null;
/**
* This sets the root user’s password on a newly-created Linode Disk when deploying from an Image.
*/
root_pass?: string;
/**
* A list of public SSH keys that will be automatically appended to the root user’s
* `~/.ssh/authorized_keys`file when deploying from an Image.
*/
authorized_keys?: string[];
/**
* If this field is set to true, the created Linode will automatically be enrolled in the Linode Backup service.
* This will incur an additional charge. The cost for the Backup service is dependent on the Type of Linode deployed.
*
* This option is always treated as true if the account-wide backups_enabled setting is true.
*
* @default false
*/
backups_enabled?: boolean;
/**
* This field is required only if the StackScript being deployed requires input data from the User for successful completion
*/
stackscript_data?: any;
/**
* If it is deployed from an Image or a Backup and you wish it to remain offline after deployment, set this to false.
* @default true if the Linode is created with an Image or from a Backup.
*/
booted?: boolean;
/**
* The Linode’s label is for display purposes only.
* If no label is provided for a Linode, a default will be assigned.
*/
label?: string;
/**
* An array of tags applied to this object.
*
* Tags are for organizational purposes only.
*/
tags?: string[];
/**
* If true, the created Linode will have private networking enabled and assigned a private IPv4 address.
* @default false
*/
private_ip?: boolean;
/**
* A list of usernames. If the usernames have associated SSH keys,
* the keys will be appended to the root users `~/.ssh/authorized_keys`
* file automatically when deploying from an Image.
*/
authorized_users?: string[];
/**
* An array of Network Interfaces to add to this Linode’s Configuration Profile.
*/
interfaces?: InterfacePayload[];
/**
* An object containing user-defined data relevant to the creation of Linodes.
*/
metadata?: UserData;
firewall_id?: number;
/**
* The `id` of the Firewall to attach this Linode to upon creation.
*/
firewall_id?: number | null;
/**
* An object that assigns this the Linode to a placment group upon creation.
*/
placement_group?: CreateLinodePlacementGroupPayload;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@linode/manager": Upcoming Features
---

Linode Create Refactor - Part 6 - Add-ons ([#10319](https://github.com/linode/manager/pull/10319))
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import React from 'react';

import { regionFactory } from 'src/factories';
import { makeResourcePage } from 'src/mocks/serverHandlers';
import { HttpResponse, http, server } from 'src/mocks/testServer';
import { renderWithThemeAndHookFormContext } from 'src/utilities/testHelpers';

import { Addons } from './Addons';

describe('Linode Create v2 Addons', () => {
it('should render an "Add-ons" heading', () => {
const { getByText } = renderWithThemeAndHookFormContext({
component: <Addons />,
});

const heading = getByText('Add-ons');

expect(heading).toBeVisible();
expect(heading.tagName).toBe('H2');
});

it('renders a warning if an edge region is selected', async () => {
const region = regionFactory.build({ site_type: 'edge' });

server.use(
http.get('*/v4/regions', () => {
return HttpResponse.json(makeResourcePage([region]));
})
);

const { findByText } = renderWithThemeAndHookFormContext({
component: <Addons />,
useFormOptions: { defaultValues: { region: region.id } },
});

await findByText(
'Backups and Private IP are currently not available for Edge regions.'
);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import React from 'react';
import { useWatch } from 'react-hook-form';

import { Divider } from 'src/components/Divider';
import { Notice } from 'src/components/Notice/Notice';
import { Paper } from 'src/components/Paper';
import { Stack } from 'src/components/Stack';
import { Typography } from 'src/components/Typography';
import { useRegionsQuery } from 'src/queries/regions/regions';

import { Backups } from './Backups';
import { PrivateIP } from './PrivateIP';

import type { CreateLinodeRequest } from '@linode/api-v4';

export const Addons = () => {
const regionId = useWatch<CreateLinodeRequest, 'region'>({ name: 'region' });

const { data: regions } = useRegionsQuery();

const selectedRegion = regions?.find((r) => r.id === regionId);

const isEdgeRegionSelected = selectedRegion?.site_type === 'edge';

return (
<Paper>
<Stack spacing={2}>
<Typography variant="h2">Add-ons</Typography>
{isEdgeRegionSelected && (
<Notice
text="Backups and Private IP are currently not available for Edge regions."
variant="warning"
/>
)}
Comment on lines +32 to +37
Copy link
Member Author

Choose a reason for hiding this comment

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

There are other notices that will need to be added here related to cloning. I will add those when I add cloning support down the road.

<Stack divider={<Divider />} spacing={2}>
<Backups />
<PrivateIP />
</Stack>
</Stack>
</Paper>
);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
import { waitFor } from '@testing-library/react';
import React from 'react';

import { accountSettingsFactory, regionFactory } from 'src/factories';
import { makeResourcePage } from 'src/mocks/serverHandlers';
import { HttpResponse, http, server } from 'src/mocks/testServer';
import { renderWithThemeAndHookFormContext } from 'src/utilities/testHelpers';

import { Backups } from './Backups';

import type { CreateLinodeRequest } from '@linode/api-v4';

describe('Linode Create V2 Backups Addon', () => {
it('should render a label and checkbox', () => {
const { getByLabelText } = renderWithThemeAndHookFormContext({
component: <Backups />,
});

const checkbox = getByLabelText('Backups', { exact: false });

expect(checkbox).toBeEnabled();
expect(checkbox).not.toBeChecked();
});

it('should get its value from the from context', () => {
bnussman-akamai marked this conversation as resolved.
Show resolved Hide resolved
const {
getByRole,
} = renderWithThemeAndHookFormContext<CreateLinodeRequest>({
component: <Backups />,
useFormOptions: { defaultValues: { backups_enabled: true } },
});

const checkbox = getByRole('checkbox');

expect(checkbox).toBeEnabled();
expect(checkbox).toBeChecked();
});

it('should render special copy, be checked, and be disabled if account backups are enabled', async () => {
server.use(
http.get('*/v4/account/settings', () => {
return HttpResponse.json(
accountSettingsFactory.build({ backups_enabled: true })
);
})
);

const { findByText, getByRole } = renderWithThemeAndHookFormContext({
component: <Backups />,
});

const checkbox = getByRole('checkbox');

await findByText('You have enabled automatic backups for your account.', {
exact: false,
});

expect(checkbox).toBeDisabled();
expect(checkbox).toBeChecked();
});

it('should be disabled if an edge region is selected', async () => {
const region = regionFactory.build({ site_type: 'edge' });

server.use(
http.get('*/v4/regions', () => {
return HttpResponse.json(makeResourcePage([region]));
})
);

const {
getByRole,
} = renderWithThemeAndHookFormContext<CreateLinodeRequest>({
component: <Backups />,
useFormOptions: { defaultValues: { region: region.id } },
});

const checkbox = getByRole('checkbox');

await waitFor(() => {
expect(checkbox).toBeDisabled();
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
import React from 'react';
import { useController, useWatch } from 'react-hook-form';

import { Checkbox } from 'src/components/Checkbox';
import { Currency } from 'src/components/Currency';
import { FormControlLabel } from 'src/components/FormControlLabel';
import { Link } from 'src/components/Link';
import { Stack } from 'src/components/Stack';
import { Typography } from 'src/components/Typography';
import { useRestrictedGlobalGrantCheck } from 'src/hooks/useRestrictedGlobalGrantCheck';
import { useAccountSettings } from 'src/queries/account/settings';
import { useRegionsQuery } from 'src/queries/regions/regions';
import { useTypeQuery } from 'src/queries/types';
import { getMonthlyBackupsPrice } from 'src/utilities/pricing/backups';

import { getBackupsEnabledValue } from './utilities';

import type { CreateLinodeRequest } from '@linode/api-v4';

export const Backups = () => {
const { field } = useController<CreateLinodeRequest, 'backups_enabled'>({
name: 'backups_enabled',
});

const isLinodeCreateRestricted = useRestrictedGlobalGrantCheck({
globalGrantType: 'add_linodes',
});

const regionId = useWatch<CreateLinodeRequest, 'region'>({ name: 'region' });
const typeId = useWatch<CreateLinodeRequest, 'type'>({ name: 'type' });

const { data: type } = useTypeQuery(typeId, Boolean(typeId));
const { data: regions } = useRegionsQuery();
const { data: accountSettings } = useAccountSettings();

const backupsMonthlyPrice = getMonthlyBackupsPrice({
region: regionId,
type,
});

const selectedRegion = regions?.find((r) => r.id === regionId);
Copy link
Contributor

Choose a reason for hiding this comment

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

I am still finding this a bit flawed. Yes, this is relatively inexpensive but because of only storing the region ID in the form state instead of the whole region object we are probably running regions.find 5 or 6 times on the Linode create flow - maybe more. Not a big deal but not the cleanest

Copy link
Member Author

@bnussman-akamai bnussman-akamai Mar 27, 2024

Choose a reason for hiding this comment

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

Yeah, this is a great point. I was considering adding a useRegion query to avoid needing to run a regions.find everywhere, but this could introduce an extra fetch.

This might be worth it considering how often we call regions.find throughout the app

Have any thoughts on that or other ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible/a good idea to break pattern and have the form state being the whole region?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that would be great. I think it would make things a ton more complicated with regarding types and validation

Copy link
Contributor

@abailly-akamai abailly-akamai Mar 27, 2024

Choose a reason for hiding this comment

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

makes sense. then i think the best option would be a useMemo util so at least we're not recalculating every time?


const isAccountBackupsEnabled = accountSettings?.backups_enabled ?? false;

const isEdgeRegionSelected = selectedRegion?.site_type === 'edge';

return (
<FormControlLabel
checked={getBackupsEnabledValue({
accountBackupsEnabled: isAccountBackupsEnabled,
isEdgeRegion: isEdgeRegionSelected,
value: field.value,
})}
disabled={
isEdgeRegionSelected ||
isLinodeCreateRestricted ||
isAccountBackupsEnabled
}
label={
<Stack sx={{ pl: 2 }}>
<Stack alignItems="center" direction="row" spacing={2}>
<Typography variant="h3">Backups</Typography>
{backupsMonthlyPrice && (
<Typography>
<Currency quantity={backupsMonthlyPrice} /> per month
</Typography>
)}
</Stack>
<Typography>
{isAccountBackupsEnabled ? (
<React.Fragment>
Copy link
Contributor

Choose a reason for hiding this comment

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

are we still doing fragments this way rather than <>?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was just copy paste from packages/manager/src/features/Linodes/LinodesCreate/AddonsPanel.tsx. I'm open to using <> if we want to prefer that

You have enabled automatic backups for your account. This Linode
will automatically have backups enabled. To change this setting,{' '}
<Link to={'/account/settings'}>click here.</Link>
</React.Fragment>
) : (
<React.Fragment>
Three backup slots are executed and rotated automatically: a
daily backup, a 2-7 day old backup, and an 8-14 day old backup.
Plans are priced according to the Linode plan selected above.
</React.Fragment>
)}
</Typography>
</Stack>
}
control={<Checkbox />}
onChange={field.onChange}
/>
);
};
Loading
Loading