Added endpoint to delete link #179
Conversation
There was a problem hiding this comment.
Nicely done @Alopalao, overall looks great, but I raised a potential race condition and a corner case that we need to handle and make a decision regarding if we'll only allow disabled links to get deleted or not.
Other than that, please explore these cases too:
- Try to delete a link that's under and active maintenance and see what happens
- Disable and delete a link with this new endpoint, but before you do it, make sure to also disable lldp on the underlying interfaces, the end result is that this link shouldn't get rediscovered again unless lldp gets enabled.
- Repeat the same but without disabling lldp, the link should get rediscovered again
Let's also add at least one new e2e test for this, it can be a simple one, but let's do it, ensuring it's no longer in the response of topology endpoints.
I explored those cases. In all of them the results were as described. |
There was a problem hiding this comment.
Overall looks good, just a minor details left to be addressed @Alopalao
Also, let me know if you had positive outcomes with the local/exploratory tests suggested on #179 (review)
Yes, I tested these tests. They all had positive outcomes. |
|
Created PR #273 for End-To-End tests |
There was a problem hiding this comment.
@Alopalao LGTM. Let's merge this.
Can you also map a new UI task for this endpoint? It'd be great to have a button to also allow deleting from the UI. Let me know if you can still deliver this minor one too, otherwise it'll be for the next version.
|
I created the issue and I think I am going to be able to finish it for this version. |
Cool. Appreciated. |
Closes #154
Summary
Added an endpoint to delete a link
Local Tests
Added unit tests
Deleted a link that was previously removed with
link s1 s3 downon mininet console.