-
Notifications
You must be signed in to change notification settings - Fork 364
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-7445] - Public IP Addresses Tooltip and LISH Display #11070
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.
I originally opened this PR as a Draft PR as I was trying to understand the acceptance criteria. Based on my understanding I believe this file should be deleted and replaced with PublicIPAddressesTooltip
, rather than renamed and updated. Before I actually delete the file I commented it out, and will await PR feedback.
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.
Seems like it would come out to basically the same either way
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.
"Public IP Addresses" tooltip copy is updated for VPC-only linodes ✅
In creating a linode to test this PR, I noticed a bug in the new Linode Create flow -- if the linode is assigned to a VPC during creation, that VPC interface should be marked as primary
. See the relevant Linode Create v1 code:
primary: true, |
That doesn't seem to be happening in the new flow as I had to resave the config after creation to trigger that setting
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.
Seems like it would come out to basically the same either way
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.
There's a potential contradiction in the ticket because the mockup shows the "Public IP Addresses" and "Access" fields as being enabled, but the description only mentions, "Update the LISH Console via SSH terminal command text to be displayed as enabled." Did UX confirm that all of the fields under those headers should be 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.
That was my confusion. I looked at the other tickets referenced in the Jira ticket and the conversation was such that we initially disabled the entire right side of the LinodeEntityDetailBody
and show the tooltip. Then it was decided to not disable the right side of the LinodeEntityDetailBody
and instead just update the text copy of the tooltip. Those are the changes done in this PR.
Both people who would have more context are OOO until Monday. I've reached out to them., we can wait to hear from them.
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.
@dwiley-akamai Please review comment and updated mock from UX in the relevant ticket for this PR.
I can create a separate bug ticket to address this issue as I can replicate it myself. |
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.
Code review ✅
Tooltip copy and URL updated ✅
"Access" section now enabled for VPC-only linode ✅
"Public IP Addresses" and "Access" sections for non-VPC-only linodes not adversely impacted by changes ✅
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.
✅ confirmed updated tooltip copy/link and Access section is enabled for vpc-only linode
✅ confirmed Public IP Addresses and Access section remain the same for non vpc-only linodes
✅ confirm additional test coverage passes + e2e test failures look unrelated
Thanks Erik!
@@ -170,6 +170,7 @@ export const LinodeEntityDetailBody = React.memo((props: BodyProps) => { | |||
)} | |||
</StyledSummaryGrid> | |||
</Grid> | |||
|
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 like some extra spacing snuck into this file (unless you meant to include it!)
Description 📝
Enable Public IP Addresses for VPC-only Linodes and updated the Tooltip text.
These changes follow the discussion in which it was acknowledged that LISH does not use a Public IP Address and this was not known at the time the original design was created.
A VPC-only Linode is a Linode that has at least one config interface with
primary
set totrue
and purposevpc
and noipv4.nat_1_1
value.Changes 🔄
Target release date 🗓️
10/14/2024
Preview 📷
How to test 🧪
Prerequisites
(How to setup test environment)
assign a public IPv4 address
checkbox is unchecked)Verification steps
(How to verify changes)
For VPC-only Linode
For non-VPC Linodes
As an Author I have considered 🤔
Check all that apply