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

feature: Added get endpoints for Interface tag ranges #166

Merged
merged 5 commits into from
Oct 11, 2023

Conversation

Alopalao
Copy link

@Alopalao Alopalao commented Sep 29, 2023

Closes #162

Summary

This is based on the branch epic/vlan_pool which is not going to be modified.
Added endpoints:

  • GET v3/interfaces/{interface_id}/tag_ranges
  • GET v3/interfaces/tag_ranges

Local Tests

Called these new API endpoints
Added unit tests

End-to-End Tests

N/A

Note:
Tox is passing

@Alopalao Alopalao requested a review from a team as a code owner September 29, 2023 16:20
@Alopalao Alopalao changed the title Added get endpoints for Interface tag ranges feature: Added get endpoints for Interface tag ranges Sep 29, 2023
Copy link
Member

@viniarck viniarck left a comment

Choose a reason for hiding this comment

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

Functionality-wise looks good. Asked a few changes to avoid thread-safety issues. Also, don't forget to update the changelog.

I'll also recommend that now that you have this endpoint to add a new e2e test covering it, I believe a least one e2e covering these points below should be great, feel free to add other ones as you wish:

Here's a suggestion for one e2e test case step (feel free to adapt or change accordingly):

  • Restrict tag ranges on a UNI interface on a ring topology
  • Try to create an EVC, confirm that it won't allow it
  • Confirm that both tag ranges endpoints are returning the expected responses
  • Add another tag range restriction that's compatible with the UNI vlan that you want to use, now the EVC should be created successfully
  • Remove the tag ranges restriction confirm the expected responses

This is a larger test to cover a more elaborated scenario, but feel free to create smaller ones to test individually, it's up to you, thanks, Aldo.

main.py Outdated Show resolved Hide resolved
main.py Outdated Show resolved Hide resolved
main.py Outdated Show resolved Hide resolved
main.py Outdated Show resolved Hide resolved
openapi.yml Show resolved Hide resolved
openapi.yml Show resolved Hide resolved
- Modified changelog
- Improved openapi
@viniarck viniarck self-requested a review October 2, 2023 19:27
Copy link
Member

@viniarck viniarck left a comment

Choose a reason for hiding this comment

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

Nicely done @Alopalao.

I'll wait for the new e2e test(s) before approving for completeness.

@viniarck viniarck self-requested a review October 3, 2023 17:01
main.py Outdated Show resolved Hide resolved
@viniarck viniarck self-requested a review October 3, 2023 19:48
tests/unit/test_main.py Outdated Show resolved Hide resolved
@viniarck viniarck self-requested a review October 3, 2023 20:03
Copy link
Member

@viniarck viniarck left a comment

Choose a reason for hiding this comment

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

Excellent PR, Aldo. Awesome how the endpoints turned out.

I'll leave it pre-approved. Feel free to merge it once Italo also approves mef_eline PR kytos-ng/mef_eline#371 when he returns to the office.

Base automatically changed from epic/vlan_pool to master October 11, 2023 13:27
@viniarck viniarck merged commit 151db27 into master Oct 11, 2023
1 check passed
@viniarck viniarck deleted the epic/vlan_pool_request_get branch October 11, 2023 13:47
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.

Add GET API requests for vlan_pool
2 participants