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

[enhancement] Add curl cacert capath options #134 #135

Merged
merged 3 commits into from
Jul 4, 2024

Conversation

SLSC1907
Copy link
Contributor

@SLSC1907 SLSC1907 commented Jul 3, 2024

Addresses #134

Update monitoring.init and monitoring.agent for curl cacert capath options from openwisp-config configuration.

Addresses openwisp#134

Update monitoring.init and monitoring.agent for curl cacert capath options
from openwisp-config configuration.
@nemesifier nemesifier added the enhancement New feature or request label Jul 3, 2024
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Looks good @SLSC1907, please add these options to the Configuration Options section in the README.

@SLSC1907
Copy link
Contributor Author

SLSC1907 commented Jul 4, 2024

Thanks for the feedback.
I didn't include it as the documentation and configuration is set from the openwrt-openwisp-config package.

i.e. These UCI options aren't read from /etc/config/openwisp-monitoring but are pulling the options from /etc/config/openwisp just like the existing verify_ssl option.

As it exists, if you use the cacert and capath options with verify_ssl in openwisp-config, the monitoring package will fail to upload / verify the certificate.

The options are documented however in the monitoring.agent printed help output, like the verify_ssl option.

Would you like all the Openwisp general config items documented in the readme for openwisp-monitoring?

My plan is to add a note underneath regarding the application also pulls configuration options from /etc/config/openwisp. I can write a separate documentation update too? Not sure how you'd like to proceed.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

@SLSC1907 you're right, documenting these options is not needed, I had missed this detail in my previous review, thanks for pointing it out. I fixed some issues with whitespace (we use tabs here instead of spaces), not it can be merged. Thanks for contributing! 🙏

@nemesifier nemesifier merged commit a174666 into openwisp:master Jul 4, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants