Introduction of Provider Networks #1144
Conversation
|
Currently, I am blocked by #1137 since I am not able to test this feature properly, I guess. |
|
Hi @johannwagner, it appears that #1137 was fixed by #1145. If you update your branch to pull in latest |
johannwagner
force-pushed
the
feature/provider-network
branch
from
14c41f6
to
206257a
Compare
|
I rebased my work and it seems to work similar to the Netbox implementation. I think, I want to keep this seperate. We want to implement @chadell idea for our own purposes but I would like to merge this PR first and add the IP Range association within another PR. |
|
What is needed to merge this? |
nautobot/circuits/templates/circuits/circuittermination_edit.html
Outdated
Show resolved
Hide resolved
nautobot/circuits/templates/circuits/circuittermination_edit.html
Outdated
Show resolved
Hide resolved
|
ToDo-List:
|
|
Okay, thanks for your reviews. I think, I covered every single review comment and we should be ready to merge 🎉 |
nautobot/circuits/models.py
Outdated
| "webhooks", | ||
| ) | ||
| class ProviderNetwork(PrimaryModel): | ||
| name = models.CharField(max_length=100, unique=True) |
There was a problem hiding this comment.
I think unique=True is incorrect here as below you're defining the unique_together = ("provider", "name") - it should be permissible to use the same name across different providers, I think?
There was a problem hiding this comment.
Since we are using this as a base for our slug, which needs to be unique, we may also want to enforce it here.
If it is no problem for the slug handling, I am happy to remove it.
There was a problem hiding this comment.
AutoSlugField is supposed to automatically handle uniqueness at creation time - IIRC if you create Provider 1 Network "Alpha" (slug alpha) and Provider 2 Network "Alpha" (slug alpha) the latter will automatically become slug alpha1 so as to have a unique slug - but by all means please try it out to confirm.
There was a problem hiding this comment.
It does not get automagically changed, but the creation gives a reasonable error.
|
Getting nearly there! Shoot - I just realized that this should be a PR against Thank you! |
|
Okay, should there be any needed changes? I switched the base branch and it seems to merge fine. I did not test it, will do tomorrow. |
|
I will also fix tests tomorrow. |
|
Unit tests work on my local machine, hopefully CI is also happy about them. |
|
Looks like the migrations file needs to be renumbered/regenerated so as not to conflict with the migration introduced in #1107. Sorry for the trouble! |
|
@jathanism Thanks for your comments. I mostly only ported the code from Netbox, so I am not that deep in all thought processes but I will do my best to resolve your concerns. |
johannwagner
force-pushed
the
feature/provider-network
branch
from
b72db83
to
1a65466
Compare
|
Okay, I added the feedback from @jathanism and rebased the branch to next for a better overview. I think, I need another CI run to make sure, everything is sorted now. |
|
I enjoyed this one! Thanks for the great community experience and your time and effort! Looking forward to ship this! |
* Initial port of netbox-community/netbox#6011 * Added tests and fixed occuring bugs by adding tests * Fixed Linting * Applied review feedback * Merged migrations * Renaming providernetwork to provider_network * Fixing weight on navigation * Added slug field for natural keys and applied feedback from review * Fixed Linting * Fixed provider network template to only overwrite sub parts * Fixing unit tests and applied feedback * Fixed up migrations after rebasing for next * Applied feedback * Fixed Linting
Fixes: #724
This pull request introduces the functionality of Provider Networks. They are known from Netbox.
TODOs: