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

migrate node network list functionality into esisdk #5

Merged
merged 1 commit into from
Jul 2, 2024

Conversation

ajamias
Copy link
Collaborator

@ajamias ajamias commented Jun 26, 2024

The python-esiclient and esi-ui packages depend on the same functionality. This PR starts to centralize that logic so no code repetition is done. The first function to be added is openstack esi node network list.

esi/lib/node.py Outdated
None, None, None, None, None])

return ["Node", "MAC Address", "Port", "Network", "Fixed IP",
"Floating Network", "Floating IP"], data
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll take a closer look tomorrow, but one thing sticks out: we want to return data, and not the column headers. I'd consider the headers part of the formatting, and that belongs in python-esiclient. Another way to think of this, is if you want to use this function in the UI, you're only going to want data.

Copy link
Collaborator

@tzumainn tzumainn Jun 26, 2024

Choose a reason for hiding this comment

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

Taking that one step forward, I wonder if this might be best returned as a list of dictionaries, where each dictionary is something like:

{
    "node": <node object>,
     "ports": [{
           "baremetal_port": <baremetal port object>,
           "neutron_port": <neutron port object>,
           "floating_ip": <floating ip information>
          },
          <additional ports>
       ]
}

Then the client gets to interpret the information however they'd like.

This would imply that some of the functions in utils.py that do formatting - like get_network_display_name - would still belong in python-esileapclient

Copy link
Collaborator

@tzumainn tzumainn left a comment

Choose a reason for hiding this comment

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

Good start! I've left comments inline.

esi/lib/node.py Outdated Show resolved Hide resolved
esi/lib/node.py Outdated Show resolved Hide resolved
esi/lib/node.py Outdated Show resolved Hide resolved
esi/utils.py Outdated Show resolved Hide resolved
esi/utils.py Outdated Show resolved Hide resolved
esi/utils.py Outdated Show resolved Hide resolved
esi/lib/node.py Outdated Show resolved Hide resolved
esi/lib/node.py Outdated Show resolved Hide resolved
esi/lib/node.py Outdated Show resolved Hide resolved
esi/lib/node.py Outdated Show resolved Hide resolved
esi/lib/nodes.py Outdated Show resolved Hide resolved
esi/lib/nodes.py Show resolved Hide resolved
esi/tests/unit/lib/test_node.py Outdated Show resolved Hide resolved
esi/tests/unit/lib/test_node.py Outdated Show resolved Hide resolved
esi/tests/unit/test_utils.py Outdated Show resolved Hide resolved
@tzumainn
Copy link
Collaborator

tzumainn commented Jul 1, 2024

Looks pretty good! A few last things:

  • there's a lint error; make sure you run tox -epep8
  • can you squash the commits? make sure the new one commit has a good comprehensive title/description
  • can you update your python-esiclient PR so I can test against this?

@ajamias
Copy link
Collaborator Author

ajamias commented Jul 1, 2024

* can you update your python-esiclient PR so I can test against this?

The python-esiclient PR doesn't have fully updated test cases, only the source code, but I'll update it right now

@tzumainn
Copy link
Collaborator

tzumainn commented Jul 1, 2024

Thanks for the update! After testing, I've noted three differences in output. They could all very well be due to presentation python-esiclient, but you may want to be sure:

  • The external network now shows its VLAN. Honestly... this is better so I'd keep this.
  • If a floating IP exposes its whole range (and not just a port forwarding), there's empty parentheses that weren't there before.
  • The tagged ports in a trunk port don't show their fixed IP addresses.
| MOC-R4PAC21U25-S1 | 0c:42:a1:7b:e4:f0 | esi-esi-MOC-R4PAC21U25-S1-trunk-tzumainn-trunk-port | tzumainn (626)  | 192.168.50.140 | external (3801)  | 128.31.20.114 () |
|                   |                   |                                                     | tzumainn2 (600) |                |                  |                  |

@ajamias ajamias force-pushed the main branch 4 times, most recently from 1b06247 to 7786956 Compare July 1, 2024 20:49
@ajamias
Copy link
Collaborator Author

ajamias commented Jul 1, 2024

Thanks for the update! After testing, I've noted three differences in output. They could all very well be due to presentation python-esiclient, but you may want to be sure:

* The external network now shows its VLAN. Honestly... this is better so I'd keep this.

* If a floating IP exposes its whole range (and not just a port forwarding), there's empty parentheses that weren't there before.

* The tagged ports in a trunk port don't show their fixed IP addresses.
| MOC-R4PAC21U25-S1 | 0c:42:a1:7b:e4:f0 | esi-esi-MOC-R4PAC21U25-S1-trunk-tzumainn-trunk-port | tzumainn (626)  | 192.168.50.140 | external (3801)  | 128.31.20.114 () |
|                   |                   |                                                     | tzumainn2 (600) |                |                  |                  |

these should be fixed now

Copy link
Collaborator

@tzumainn tzumainn left a comment

Choose a reason for hiding this comment

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

This looks really good! The one thing missing is in the test cases, which should have assert_called and assert_not_called (as in https://github.com/CCI-MOC/esi-leap/blob/master/esi_leap/tests/api/controllers/v1/test_utils.py#L262). The reason is that this is often a way to ensure that conditional branches are followed correctly.

Once that's included, I think this will be ready to merge!

esi/tests/unit/lib/test_nodes.py Outdated Show resolved Hide resolved
Originally, this functionality existed in python-esiclient. We moved
it into esisdk so that multiple projects can reuse the same package,
and to centralize the logic in one place.
@tzumainn
Copy link
Collaborator

tzumainn commented Jul 2, 2024

Looks good - thanks!

@tzumainn tzumainn merged commit af9c65a into CCI-MOC:main Jul 2, 2024
6 checks passed
@ajamias ajamias linked an issue Jul 23, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update sdk with logic used in python-esiclient
2 participants