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

Add linknet PUT method and clean up linknet API a bit #243

Merged
merged 7 commits into from
Feb 16, 2022

Conversation

indy-independence
Copy link
Member

Allow update/PUT on linknets
Allow GET a single linknet
Update DELETE to use same path/syntax as other parts of API

@indy-independence indy-independence added the enhancement New feature or request label Jan 26, 2022
@indy-independence indy-independence self-assigned this Jan 26, 2022
@codecov
Copy link

codecov bot commented Jan 26, 2022

Codecov Report

Merging #243 (5c065c5) into develop (51adce6) will increase coverage by 1.01%.
The diff coverage is 72.35%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #243      +/-   ##
===========================================
+ Coverage    59.80%   60.81%   +1.01%     
===========================================
  Files           62       62              
  Lines         6229     6331     +102     
===========================================
+ Hits          3725     3850     +125     
+ Misses        2504     2481      -23     
Impacted Files Coverage Δ
src/cnaas_nms/api/generic.py 72.26% <50.00%> (-0.39%) ⬇️
src/cnaas_nms/api/linknet.py 66.49% <72.03%> (+46.92%) ⬆️
src/cnaas_nms/api/app.py 68.69% <100.00%> (+0.27%) ⬆️
src/cnaas_nms/db/linknet.py 88.40% <0.00%> (+8.69%) ⬆️
src/cnaas_nms/confpush/underlay.py 35.29% <0.00%> (+15.68%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51adce6...5c065c5. Read the comment docs.

@indy-independence indy-independence marked this pull request as ready for review January 26, 2022 14:22
@indy-independence
Copy link
Member Author

Not sure how to add external users as reviewers but if you have some input @lunkwill42 @stveit go ahead!
Addresses some similar concerns as #207

@stveit
Copy link
Contributor

stveit commented Jan 26, 2022

We might have discussed this before in general regarding the API, but is there a need to have endpoints for both linket and linknetS? If I post something to api/linknets and it gets id=1, I would expect to be able to access it from api/v1.0/linknets/1, not api/v1.0/linknet/1 as it currently is. i.e I would expect it to become a resource under the endpoint I posted to. In general i think avoiding double endpoints would streamline the API more.

Another thing that I have thought about before is the use of the PUT method. If I've understood correctly you mainly use PUT as a way to update a resource, not replacing it. For example you allow someone to use PUT to only change one field of an object. The general understanding I have of PUT is that it should be used to effectively replace an object with a modified version (so for a linknet you would define the entirety of the linket object in the PUT message, needing all required fields to be set) , while PATCH can be used to change individual fields (ex. only update port). I am not trying to say that your approach is wrong, but I wonder if this distinction between PUT and PATCH is something you guys have thought about?

Ive just scimmed the changes a little so this is all I have to say for now, I'll look more in depth on friday :)

@indy-independence
Copy link
Member Author

Ah yeah the discussion on linknet vs linknetS etc has come up before, so I guess it's something people care about. I think it's mostly personal preference if you like to have same URL or URL that sounds more like natural language. One suggestion could be to just add all single item paths on to the plural paths as well, so if you like to always have the S in the end it will actually work, but maybe docs stay the same for now and maybe just a note you can add ending S optionally? Either way the most important thing for me consistency, which we don't really have today :)

I haven't thought about PUT vs PATCH, it sounds pretty logical they way you say it but I can't remember that I used any APIs that made a distinction and provided both PUT and PATCH.

@stveit
Copy link
Contributor

stveit commented Feb 7, 2022

Busy week so been a bit slower than i wanted to be.
Good change to delete, makes a lot of sense.
Might not be a use case for this, but could also allow to delete all linknets by
sending delete request to api/linknets.

Could linknet_device_ip_validator in linknet.py be put inside the f_linknet model? It doesnt seem to be used anywhere else. And just slap @validator('device_a_ip', 'device_b_ip') on it so you can remove the _device_a_ip_valid and _device_b_ip_valid fields.
I also think you could consider putting the pydantic model in its own file, src/cnaas_nms/api/models/f_linknet.py or something.

@indy-independence
Copy link
Member Author

Ah yes @validator('device_a_ip', 'device_b_ip') seems to work great! I guess I never realized you can put two arguments there, instead I did that roundabout way with _device_a_ip_valid field and allow_reuse etc :P

@indy-independence indy-independence merged commit 53e75c1 into develop Feb 16, 2022
@indy-independence indy-independence deleted the feature.linknet_put branch February 16, 2022 13:15
@indy-independence indy-independence added this to the v1.4.0 milestone Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants