-
Notifications
You must be signed in to change notification settings - Fork 19
Replace Capirca dependency with latest Aerleon (new PR) #308
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
base: develop
Are you sure you want to change the base?
Conversation
|
Wow, surprised to see this in my inbox this morning. It looks good but I am not overly familiar with the code base. |
|
Summary of supplementary changes:
Forward and backward database migrations are OK as far as I can see (tested today with small non-prod data set). |
|
Edited PR description to reflect new changes |
|
Finally looked into this. This would be a breaking change.. which I am okay with. That being said need to consider to following for a smooth transition:
Also, it would be good to understand what type of testing you have done. |
Regarding testing, we did the following things:
Currently we're still in the testing phase for Firewall Models App, and we do not have any live, production deployments built on top of it atm. This change is part of work we're doing to adapt it to our specific requirements. I have already contributed at Aerleon for Proxmox firewall support and helped Rob (Aerleon maintainer) for Fortigate support (part of our requirements). |
|
I just added the configuration checks for leftover capirca configuration. Would you mind explaining |
|
The code changes look sane. I think need to consider id we make the name generic? I somewhat regret calling it Capirca to begin with, for this exact reason. For custom integration, see the below from these docs |
We could make it generic but also I don't think we're that likely to change backend again soon(?). And I would consider it ok having to do a big refactor/renaming only one time. The reason we proposed Aerleon was because, for us, it was way easier to get features upstreamed compared to Capirca. With the Aerleon backend under NTC umbrella maintenance should be easier IMO. I'll let you decide.
Thanks! |
|
Spoke with @jdrew82 we have some thoughts. Just need to finalize with @cmsirbu, which I will setup time to meet next week. In the meantime:
|
|
ok thx @itdependsnetworks. do you know if there will be room to also squeeze #316 in the next release? |
|
Before a major release will for sure get that in. |
6832032 to
abcc3a5
Compare
|
Finally got around to it, but internally we decided to support both capirca and aerleon in a generic model, defaulting to Aerleon. Both will be able to co-exists before we sunset the current way of doing it. Appreciate all of your efforts and it was helpful to review this PR as making all of the changes See PR: #337. |
|
Ok, I understand wanting to let the users choose rather than deciding for them. I'll check if we can still provide work on #316 (which is still open on your side afaik), given it was dependent on this exact PR and will very likely require rework. |

aerleonto latest release.Rebased upon latest develop and solved conflicts.
I had to do some minor changes from previous PR (JSON Schema and some class property defaults which were different).
I'll test this in our lab and report if we have any issues for config generation from the app.
cc @ankenyr