-
Notifications
You must be signed in to change notification settings - Fork 10
Allow external redis and and mysql #94
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
Conversation
jacobw
left a comment
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.
Thanks for the PR. I've started a review but will need to continue it as time permits.
|
Are you able to split this PR in to commits for each part. of the change. I.e. lock file, external redirects and mysql, health checks, mount removals. This is partly to make the review easier but also to make the commit history easy to follow for others. |
|
Sorry, I normally don't use Github so not sure on how to do multiple PR's using forks. I have removed all other changes and I hope that I remember to make new PR's after this one is merged. |
|
You can submit multiple commits in the PR, they don't have to be separate PRs. This PR as it is has many commits that would pollute the commit history of the repo if the PR was merged so they need to be cleaned up in advance. if you're really stuck, just create a fresh fork and create some clean commits. If you get your local repo into a good state you can force push and it will update the PR automatically. I usually use interactive rebases to clean up individual commits on a PR. |
|
Ahh, I misread. I rebased it into a single commit and I'll add commits for the rest |
Use value librenms.timezone for rrdcached timezone instead of seperate environment variable setting
|
While looking up the changes I found an extra error in case But I think this is it. The commits look good and precise to me and the configuration works in my environment. |
|
Thanks - looking much better! I'll continue the review as soon as I can. |
|
I have created a separate PR that added all of the changed unrelated to the external mysql and redis. I'm open to any PRs to add this now. |
|
I'm sorry, I've no recollection of what I did 3 months ago. I'm not sure what you mean, do you mean you want me to open a new PR containing only the first commit? |
|
Hey guys, Is there any update on this? |
|
I think his PR needs quite a bit of work. I'm keen to make it happen but haven't had the time yet sorry. |
|
superseded by #177 |
Hi,
This PR is for the functionality mentioned in #18. In total I made the following changes:
Some of these are probably breaking changes but I have this chart running in my own infrastructure since just now and it seems to run fine. My
values.yaml: