-
Notifications
You must be signed in to change notification settings - Fork 647
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
Routing #88
Routing #88
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very cool!
just added some general packaging things. we can pull this any time, plugins on master stay hidden as "os-quagga-devel" during release builds. just let us know when you are ready for a first merge.
net/routing/Makefile
Outdated
| PLUGIN_VERSION= 0.0.1 | ||
| PLUGIN_COMMENT= Plugin to manage the quagga routing daemon software which offers routing protocols like RIP, IS-IS and OSPF to guide your packets through the sea | ||
| PLUGIN_MAINTAINER= franz.fabian.94@gmail.com | ||
| PLUGIN_DEVEL = devel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is automatic :)
net/routing/Makefile
Outdated
| @@ -0,0 +1,7 @@ | |||
| PLUGIN_NAME= routing | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name by service name: quagga
net/routing/Makefile
Outdated
| @@ -0,0 +1,7 @@ | |||
| PLUGIN_NAME= routing | |||
| PLUGIN_VERSION= 0.0.1 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0.1 is enough... we can go up to 0.9999999 if we want -- for pkg and release management this doesn't really matter as updates are updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like 3 number version strings - It is just a question of taste ;)
Lets keep 0.1 for the first public release and 1.x for the first release with breaking changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough. it's up to you. :) I just want to share the relevant info: users have voiced concerns about long version numbers early in OPNsense 15.1.x and we also have port revisions to deal with smaller changes here (PORTREVISION=1 etc.)
| @@ -0,0 +1,48 @@ | |||
| <?php | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also named "quagga"
| 'start' => array('quagga start'), | ||
| 'stop' => array('quagga stop'), | ||
| ), | ||
| 'name' => 'Routing', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quagga :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is that really required? I want the plugin to be called routing as it handles the routing stuff and want to keep the name quagga out of the GUI. Even if it is used in the URLs (should have used routing as the package name as well...).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this is a plugin that depends on a specific daemon. core services could be called "routing", but it could also be highly confusing for users. Plus the next plugin based on OpenBGPD or OpenOSPFD would have to go a different route in naming, we can't use "Routing2" here. Same rules for all potential plugins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, will rename it
| ! | ||
| ! | ||
| {% if helpers.exists('OPNsense.quagga.ospf.interfaces.interface') %} | ||
| {% for interface in OPNsense.quagga.ospf.interfaces.interface %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fichtner @AdSchellevis any idea why this line is unreliable? When I have multiple interfaces configured, this will be an array of interfaces as expected but when there is a single interface, this becomes the first one (unexpected if it is an array type field in the model).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fabianfrz you can wrap it in helpers.toList() like https://github.com/opnsense/core/blob/5f7fa5900d6b71f57d38998c5c8bd334955ec864/src/opnsense/service/templates/OPNsense/IDS/rule-updater.config#L7 when a migration is provided the outer "if" could be removed as well, because the structure should be created after install (but there are more similar cases in the rest of our code, it won't hurt)
net/routing/Makefile
Outdated
| PLUGIN_VERSION= 0.0.1 | ||
| PLUGIN_COMMENT= Plugin to manage the quagga routing daemon software which offers routing protocols like RIP, IS-IS and OSPF to guide your packets through the sea | ||
| PLUGIN_MAINTAINER= franz.fabian.94@gmail.com | ||
| PLUGIN_DEVEL = devel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is automatic on master branch for builds. do you want to merge this for 17.1.3? it will only be available under opnsense-devel package installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually not so bad, it allows us to merge the code to stable/17.1 without building it. It's better for review. For style the argument should be "yes". 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the value is now changed to yes
|
@fichtner I think it can be merged for now (for first tests) |
|
If you want to rename the directory, you can do that if you like |
|
Merged, thanks! |
|
@fichtner Let's see how long it takes until somebody will blame me for bad web design ;) |
|
you'll do just fine, no worries at all. I am happy for this plugin. :) |
This is experimental but I make that available for extension / discussion purpose.
Thanks to @AdSchellevis for the proxy code which is a good template for own projects.
Note: The template still may require changes to work on other devices than an APU1d4. It may be a good idea to check it with the interface names removed.