-
Notifications
You must be signed in to change notification settings - Fork 352
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
change: [M3-6838] - Better "Backups Enabled" default when cloning Linode #10959
base: develop
Are you sure you want to change the base?
change: [M3-6838] - Better "Backups Enabled" default when cloning Linode #10959
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than calling setValue
in a useEffect, we can set the value at the time the Linode is selected
I think this code will need to be updated:
manager/packages/manager/src/features/Linodes/LinodeCreatev2/shared/LinodeSelectTable.tsx
Lines 107 to 116 in 75c5986
const handleSelect = async (linode: Linode) => { | |
const hasPrivateIP = linode.ipv4.some((ipv4) => privateIPRegex.test(ipv4)); | |
reset((prev) => ({ | |
...prev, | |
backup_id: null, | |
linode, | |
private_ip: hasPrivateIP, | |
region: linode.region, | |
type: linode.type ?? '', | |
})); |
We'll also want to update the form's initial values function to account for initializing values when the user deep links with a Linode preselected
export const defaultValues = async ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Confirmed that backups were enabled automatically when selecting a Linode that backups enabled ✅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks Purvesh 🚀
✅ confirmed cloning a linode with backups enabled = backups is checked and cloned linode has backups enabled
✅ confirmed cloning a linode without backups = backups is unchecked and cloned linode doesn't have backups
Unrelated to this PR - I had Managed enabled, which automatically creates backups for Linodes (and backups can't be disabled while you have Managed) - should we consider creating tickets to
for when Managed is enabled? (edit: + maybe a tooltip explaining why they're disabled)
Ah apologies I should have specified 😅 - I was referring to the checkbox not being checked when creating a Linode. Everything works as expected when cloning 🎉 |
Yes, Everything works as expected when cloning.
However, when 'Managed' is enabled, checkbox will always remain checked, right? Take note of the time when you uncheck it and click the 'Create Linode' button - it still clones the Linode with backup enabled (which is expected). I thought checked checkbox should be disabled when Managed is enabled. |
messaging you off github to continue chatting! |
e805a81
to
950a6e2
Compare
Coverage Report: ✅ |
Description 📝
Changes 🔄
Target release date 🗓️
N/A
How to test 🧪
Reproduction steps
Verification steps
As an Author I have considered 🤔
Check all that apply