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(RHINENG-4796): Add Public IP & FQDN to Infrastructure Card in Host Details Page #2130

Merged

Conversation

LiorKGOW
Copy link
Member

@LiorKGOW LiorKGOW commented Dec 22, 2023

Description

Add Public IP & FQDN to Infrastructure Card in Host Details Page
The necessary changes that address the issue described on the ticket

Associated Jira ticket: #RHINENG-4796

How to test the PR

Ensure a host containing the properties public_ipv4_addresses (& public_dns) is presenting those properties on its Infrastructure Card in its Host Details page
(It could also be with two different hosts that each contain one property)

Expected Functionality:

  • Hosts that have a property, should present it.
  • Hosts that do not have a property, should present 'Not available' instead.

To test, you can search for a host named lior_test_ip-172-31-89-28.ec2.internal on insights-qa account.
This host contains the necessary public_ipv4_addresses property that should be presented on the Infrastructure Card.

Please Read

However, because we are not yet collecting the public_dns, we can not create a host that contain this property.
Because both of these properties are part of the same SystemProfile object that is received in the relevant API response, both properties have the same implementation in this PR, which should work.

Here is a link to the scheme of SystemProfile that is being returned:
https://github.com/lzap/inventory-schemas/blob/fe94ba7ab5561abf1e1d644645b0110d6e64fbc0/schemas/system_profile/v1.yaml#L389-L406

The API endpoint that receives SystemProfile that SHOULD contain both properties, but now containing only 1 because this host does not contain the public_dns property (see the screenshots attached):

Screenshot from 2024-01-08 15-18-50

Screenshot from 2024-01-08 15-19-06

Tested locally that when a host has also the public_dns property, it is being presented in the UI alongside the public_ipv4_addresses property.
Attached screenshots of the changes and the UI:

Added code that is needed to ensure that the necessary values are presented when received

Added code that is needed to ensure that the necessary values are presented when received

The passed values are presented in the UI

The passed values are presented in the UI

Before the change

Host Details Page for a host that contains the public_ipv4_addresses property
Green - name of host so we can easily find it
Yellow - Infrastructure Card does not contain the mentioned properties

Before: Host Details Page for a host that contains the public_ipv4_addresses property

After the change

Host Details Page for a host that contains the public_ipv4_addresses property
Green - name of host so we can easily find it (same host name)
Orange - New Public IP property that is showing on the Infrastructure Card because this host contains the necessary property
Yellow - New FQDN property is being presented on the card, but is showing Not Available for this host because it does not contain the necessary public_dns property

After: Host Details Page for a host that contains the public_ipv4_addresses property

Checklist

  • Check the property public_ipv4_addresses
  • Check the property public_dns -> Need to create a host containing this prop (blocked) but tested locally (explained in the Please Read section)
  • update appropriate snapshots
  • Add screenshots

@LiorKGOW LiorKGOW requested a review from a team as a code owner December 22, 2023 00:02
@LiorKGOW LiorKGOW marked this pull request as draft December 22, 2023 00:02
@LiorKGOW LiorKGOW force-pushed the host-details-add-public-ip-fqdn branch 3 times, most recently from f315ac1 to 9185ef6 Compare January 7, 2024 14:04
@LiorKGOW LiorKGOW changed the title temp Feat(RHINENG-4796): Add Public IP & FQDN to Infrastructure Card in Host Details Page Jan 7, 2024
@LiorKGOW LiorKGOW force-pushed the host-details-add-public-ip-fqdn branch from 9185ef6 to 6b65829 Compare January 7, 2024 16:47
@LiorKGOW LiorKGOW self-assigned this Jan 7, 2024
@LiorKGOW LiorKGOW force-pushed the host-details-add-public-ip-fqdn branch 4 times, most recently from 9aa88d0 to c904fc8 Compare January 8, 2024 13:07
@LiorKGOW LiorKGOW marked this pull request as ready for review January 8, 2024 13:13
@LiorKGOW LiorKGOW force-pushed the host-details-add-public-ip-fqdn branch 4 times, most recently from f3ef470 to ef1e5b9 Compare January 9, 2024 17:15
@codecov-commenter
Copy link

codecov-commenter commented Jan 9, 2024

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (c0536f0) 57.19% compared to head (700ccdd) 57.19%.
Report is 2 commits behind head on master.

Files Patch % Lines
...neralInfo/InfrastructureCard/InfrastructureCard.js 62.50% 6 Missing ⚠️
src/api/api.js 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2130      +/-   ##
==========================================
- Coverage   57.19%   57.19%   -0.01%     
==========================================
  Files         194      194              
  Lines        6223     6237      +14     
  Branches     1723     1736      +13     
==========================================
+ Hits         3559     3567       +8     
- Misses       2664     2670       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LiorKGOW
Copy link
Member Author

LiorKGOW commented Jan 9, 2024

Thank you @gkarat for helping me with this PR !
It is now ready for review ✌🏼

@LiorKGOW LiorKGOW added the enhancement New feature or request label Jan 9, 2024
Copy link
Contributor

@gkarat gkarat left a comment

Choose a reason for hiding this comment

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

Everything looks good to me! However, I believe we better no to touch value for the IPv4, IPv6, and Interfaces/NICs fields. The problem is that if any of the arrays for this fields is empty, they will render "Not available addresses" text which is not correct. It must show "0 addresses" IIRC.

image

So let's just focus on the newly added fields (DNS, Public IP), update test snapshots, and, if would be necessary in the future, change the value logic for other fields in a separate PR.

@LiorKGOW LiorKGOW force-pushed the host-details-add-public-ip-fqdn branch from ef1e5b9 to 700ccdd Compare January 11, 2024 08:52
@LiorKGOW
Copy link
Member Author

Thank you for the review @gkarat !

I have removed the second commit like you requested
The PR is now ready for another review 👍🏼

Copy link
Contributor

@gkarat gkarat left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you @LiorKGOW

@gkarat gkarat merged commit 8fd0b1e into RedHatInsights:master Jan 11, 2024
1 of 2 checks passed
@gkarat
Copy link
Contributor

gkarat commented Jan 11, 2024

🎉 This PR is included in version 1.64.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@LiorKGOW LiorKGOW deleted the host-details-add-public-ip-fqdn branch January 21, 2024 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants