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

feat: [M3-8017] - Support Linodes in Distributed Compute Regions on Image Create #10544

Conversation

bnussman-akamai
Copy link
Member

@bnussman-akamai bnussman-akamai commented Jun 4, 2024

Description 📝

  • Shows a notice saying This Linode is in a distributed compute region. Images captured from this Linode will be stored in the closest core site. if you select a Linode that is in a distributed region on the Image Create page ℹ️
  • Shows helper text saying Image will be stored in the closest core site to (us-den-edge-1) below the Linode select if the selected Linode is in a distributed region ℹ️

These changes are based on the Image Service Gen2 Figma wireframes 🎨 - Ask me for a link if you want to reference it.

Preview 📷

Before After
Screenshot 2024-06-04 at 1 53 39 PM Screenshot 2024-06-04 at 1 52 41 PM

How to test 🧪

Prerequisites

  • Have a Linode in a distributed region on your account

Verification steps

  • Go to the Image Create page (http://localhost:3000/images/create/disk)
  • Select a Linode that is in a distributed region
  • Verify you see the notice and helper text that UX reflects in the mockups 👁️🎨

As an Author I have 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

@bnussman-akamai bnussman-akamai added the Images Relating to Images label Jun 4, 2024
@bnussman-akamai bnussman-akamai self-assigned this Jun 4, 2024
@bnussman-akamai bnussman-akamai changed the title feat: [M3-8017] - Support Edge Linodes on Image Create feat: [M3-8017] - Support Linodes in Distributed Compute Regions on Image Create Jun 4, 2024
@bnussman-akamai bnussman-akamai marked this pull request as ready for review June 4, 2024 19:44
@bnussman-akamai bnussman-akamai requested a review from a team as a code owner June 4, 2024 19:44
@bnussman-akamai bnussman-akamai requested review from dwiley-akamai and cpathipa and removed request for a team June 4, 2024 19:44
await findByText('Image scheduled for creation.');
});

it('should render a notice if the user selects a Linode in a distributed compute region', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Unit test for this PR's addition

);
});

it('should render an encryption notice if disk encryption is enabled and the Linode is not in a distributed compute region', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Unit test for @dwiley-akamai 's change in #10521

Copy link
Member Author

@bnussman-akamai bnussman-akamai Jun 4, 2024

Choose a reason for hiding this comment

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

Oops, I just realized you covered this in a cypress test already. I'll keep it here anyway unless there are any objections.

Copy link
Contributor

@hkhalil-akamai hkhalil-akamai left a comment

Choose a reason for hiding this comment

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

Verified the notice appears when selecting a Linode in a distributed region.

This Linode is in a distributed compute region. Images captured
from this Linode will be stored in the closest core site.
</Notice>
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we increase the margins above and below this notice to match the mockups?

This PR Figma mockup
Screenshot 2024-06-06 at 5 02 30 PM Screenshot 2024-06-06 at 5 02 07 PM

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 increased overall spacing in ba513a7, let me know if that looks okay to you! 👁️

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding unit tests for this component! 🙌🏽

Copy link

github-actions bot commented Jun 10, 2024

Coverage Report:
Base Coverage: 82.76%
Current Coverage: 82.76%

@bnussman-akamai bnussman-akamai added the Add'tl Approval Needed Waiting on another approval! label Jun 10, 2024
@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Jun 11, 2024
@bnussman-akamai bnussman-akamai merged commit 3843169 into linode:develop Jun 12, 2024
18 checks passed
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! Images Relating to Images
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants