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

fix: [M3-6510] - Modifying Configuration Profile in Cloud breaks with VLANs #9053

Merged
merged 12 commits into from
Apr 26, 2023

Conversation

cpathipa
Copy link
Contributor

@cpathipa cpathipa commented Apr 25, 2023

Description 📝

Fix - Modifying Configuration Profile in Cloud breaks with VLANs

Preview 📷

Include a screenshot or screen recording of the change

Before After
image image

How to test 🧪

How to reproduce the issue (if applicable)?

  • Checkout to develop branch
  • Create a Linode in Cloud Manager with a standard image (Ubuntu, for example) and no VLAN
  • After the Linode provisions, edit the configuration profile in Cloud Manager, adding a VLAN (with or without an IPAM address), then Save the configuration profile
  • Edit the configuration profile again, then try to either a) add another VLAN, b) remove the existing VLAN by switching eth1 to None, or c) do literally nothing other than immediately Save the configuration profile (with no changes made).

How to verify changes?

  • Follow the above steps to create Linode, configure and update with VLAN

How to run Unit or E2E tests?

  • yarn test
  • yarn cy:run

@cpathipa cpathipa self-assigned this Apr 25, 2023
@cpathipa cpathipa changed the title M3 6510 fix: [M3-6510] - Modifying Configuration Profile in Cloud breaks with VLANs Apr 25, 2023
@cypress
Copy link

cypress bot commented Apr 25, 2023

Passing run #3197 ↗︎

0 151 3 0 Flakiness 0

Details:

PR feedback
Project: Cloud Manager E2E Commit: c7bb561bf5
Status: Passed Duration: 13:58 💡
Started: Apr 26, 2023 5:16 PM Ended: Apr 26, 2023 5:30 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

Curious as to what made this break out of nowhere 🤔

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>
@hkhalil-akamai
Copy link
Contributor

hkhalil-akamai commented Apr 25, 2023

Curious as to what made this break out of nowhere 🤔

It appears to be related to API ticket ARB-3629. The interface was modified to add the id field but the docs have not been updated.

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.

Tested to confirm the fix is working as intended.

Approved pending api-v4 suggestion by @bnussman-akamai.

Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

I think the next steps for this PR would to be to update the api-v4 types like the following

export interface Interface {
  id: number;
  label: string | null;
  purpose: InterfacePurpose;
  ipam_address: string | null;
}

type InterfacePayload = Omit<Interface, 'id'>;

Then update LinodeConfigCreationData to use InterfacePayload

CHANGELOG.md Outdated Show resolved Hide resolved
cpathipa and others added 3 commits April 26, 2023 09:34
@cpathipa
Copy link
Contributor Author

Great feedback! Thank you @bnussman-akamai and @hkhalil-akamai for taking a deeper look into this PR.

@cpathipa cpathipa requested a review from dwiley-akamai April 26, 2023 16:36
@cpathipa cpathipa merged commit b348530 into linode:develop Apr 26, 2023
@cpathipa cpathipa deleted the M3-6510 branch April 26, 2023 16:58
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.

4 participants