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

www/caddy: include os-caddy into OPNsense plugins #3840

Merged
merged 67 commits into from
Mar 8, 2024
Merged

www/caddy: include os-caddy into OPNsense plugins #3840

merged 67 commits into from
Mar 8, 2024

Conversation

Monviech
Copy link
Member

Refer to issue: #3838

I thought about a way to provide this as easily as possible as pull request, my eyes fell on the "vendor" folder.

Since I have made an own Namespace as stated in the documentation, it looked like the right place for the source code under my vendor name. Refer to: https://docs.opnsense.org/development/examples/helloworld.html#model "You should name this location with your vendor and module name..."

Thank you. ^^

Monviech and others added 2 commits February 25, 2024 16:36
@fichtner
Copy link
Member

Hi, the vendor space is for partnership purposes to add third party repositories only. The directory structure is also just 2 directories deep to be compatible with FreeBSD ports.

i think caddy should live under www/caddy to align with the FreeBSD port also.

Cheers,
Franco

@Monviech
Copy link
Member Author

i think caddy should live under www/caddy to align with the FreeBSD port also.

Cheers, Franco

Thanks for the explanation I will change the path.

Monviech and others added 6 commits February 25, 2024 18:10
Remove stray copyright
Removed reference to external documentation
Change dependency to caddy-custom to align with binary name in opnsense repo.
….d caddy script that's provided by the caddy-custom.pkg. It hooks into the onestart/onestop/onerestart actions and executes the custom setup.sh script of caddy whenever the opncaddy service is started. Additionally, all references of caddy in the service files, action files have been replaced with this opncaddy wrapper script. Due to this change, the current caddy rc.d script from the caddy-custom can be used unaltered while calling it from the opncaddy wrapper script.
…sting rc.d caddy script that's provided by the caddy-custom.pkg. It hooks into the onestart/onestop/onerestart actions and executes the custom setup.sh script of caddy whenever the opncaddy service is started. Additionally, all references of caddy in the service files, action files have been replaced with this opncaddy wrapper script. Due to this change, the current caddy rc.d script from the caddy-custom can be used unaltered while calling it from the opncaddy wrapper script."

This reverts commit 70b963a.
www/caddy/LICENSE Outdated Show resolved Hide resolved
…e location in plugin.inc since the path changed when using the standard rc.d service script.
…e location in plugin.inc since the path changed when using the standard rc.d service script. Removed custom rc.d service script.
@Monviech
Copy link
Member Author

Monviech commented Mar 6, 2024

@AdSchellevis @fichtner I have changed the Namespace and Model mount completely to OPNsense. I won't write a migration script for this. I have tested it and things seem to work as expected. Since there were a lot of changes now, it would be good if somebody else could build the package from this and test it too.

@AdSchellevis
Copy link
Member

@Monviech best keep the <mount/> tag as it was, you will loose existing data after installation when the model is saved.

@Monviech
Copy link
Member Author

Monviech commented Mar 6, 2024

Why will the data be lost, its still there, just under a different namespace in the config.xml. When this version is installed, a new caddy under the OPNsense namespace will appear, without any data. But the caddy in the Pischem namespace will still have all the old data.

If not everything is changed now, its not in line with all the other plugins and it will be always weird later.

@AdSchellevis
Copy link
Member

The data itself won't go, you just can't access it anymore with your plugin :) (and your template likely won't try to read it when re-generating as these should point to the new location as well). My suggestion was only to ease the transition, moving the config namespace with a migration can be done later as well.

I prefer the OPNsense namespace, but for the model I'm ok with leaving it as is in a first version. You can choose your strategy, I'm merely trying to make sure you have all data to choose wisely ;)

@Monviech
Copy link
Member Author

Monviech commented Mar 6, 2024

@AdShellevis if the goal is that all data has to be retained, then I will sit down and write a Migration script for it. Rather now than later when the impact would be greater. Having the framework right in the beginning should be the better choice?

@fichtner
Copy link
Member

fichtner commented Mar 6, 2024

Usually divide and conquer is the better approach. The scope of this PR was to bring it in so no need to overcommit on the goal. I’d also vote to leave the data mounted where it is now and deal with the next phase after a bit of stabilization in 24.1.4/5.

@Monviech
Copy link
Member Author

Monviech commented Mar 6, 2024

@fichtner Okay I will revert these last commits tomorrow and make sure the current model stays mounted.

…lates, migration scripts, model mount, plugin.inc, configuration sections. Everything that interacts with model and the namespace uses OPNsense\Caddy.
@Monviech
Copy link
Member Author

Monviech commented Mar 7, 2024

@AdSchellevis @fichtner Okay I think the latest commit does the trick. Everything configuration related interacts with the old config section, and everything else with the OPNsense\Caddy namespace. I would like to freeze changes here for now and want to continue from a new good known state once this has been merged. Thanks for the feedback on how to tackle this.

Use integrated php 8.0 function str_ends_with() instead of the haystack helper function.
removed '$this->' because 'str_ends_with()' is not a method of the 'Caddy' class, but a global PHP function.
I have pushed a plugin revision 1 in my other repository due to a hotfix, that's why OPNsense will have the revision 2 so it will be preferred in the future.
@fichtner fichtner linked an issue Mar 8, 2024 that may be closed by this pull request
3 tasks
@@ -0,0 +1,205 @@
# Caddy Plugin for OPNsense
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just as a side node this fits rather well for a page in the docs repo instead of dangling here in the repo but it's not a problem. just something for the wishlist.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes my plan is to write a doc page too eventually.

Copy link
Member

@fichtner fichtner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ready for merge? it's magic friday! ;)

@fichtner fichtner changed the title Include os-caddy into OPNsense plugins www/caddy: include os-caddy into OPNsense plugins Mar 8, 2024
@fichtner fichtner merged commit 67fb503 into opnsense:master Mar 8, 2024
@fichtner
Copy link
Member

fichtner commented Mar 8, 2024

merged, thanks!

@gspannu
Copy link

gspannu commented Mar 8, 2024

👍 to all...
You guys are a great bunch working on this and making OPNsense a truly powerful software with these plugins.
Kudos to all who put the effort in for this voluntary work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Please include os-caddy-plugin into OPNsense plugins
6 participants