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

Install docs, note proxy config #9720

Merged
merged 13 commits into from Feb 13, 2019

Conversation

Projects
None yet
3 participants
@mhzgh
Copy link
Contributor

commented Jan 23, 2019

As discussed here:
#9709

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

@CLAassistant

This comment has been minimized.

Copy link

commented Jan 23, 2019

CLA assistant check
All committers have signed the CLA.

@murrant

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

How do you ensure that?

@mhzgh

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2019

I thought about it a moment but decided not to give a specifc example. In the most cases this should be possible by preceding the actual command with a source invocation in the cron job. E.g. . ". ~/.profile; daily.sh;" But this assumes the users personals files were "skel-created" during the user creation. This could be blocked, when the home directory already existed before the users was created. Or maybe there are other mechanism which inject http_proxy variable. Also settings could reside in profile.d folder, but are not sourced and so on... Was not too sure how to give an most-case-true example here... IMO once there is a hint to keep that in mind, users could review the place their settings come from and act accordingly - what do you think?

@murrant

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

for http_proxy, you can set it in config.php and I think it is exported for git commands, not sure though. If it is not, it could be done.

@murrant

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

Yeah, I can't think of a silver bullet to solve the env in cron. That is why I asked ;)

@mhzgh

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2019

for http_proxy, you can set it in config.php and I think it is exported for git commands, not sure though. If it is not, it could be done.

That does sound even better to me, right. Daily.sh hands over to php anyways, and the whole application is php - so it doesnt need to be the cron-only solution.

Maybe a hint to review/set these variables during the Web-UI setup makes more sense in that case? Maybe even a lookup-check for validate.php?

Oh and https_proxy should be hitting here, to be precise - at least was in my case.

@murrant

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

A note about setting the proxy in config.php to ensure it works during cron or setting the env for cron would be good. You can link to https://docs.librenms.org/Support/Configuration/#proxy-support

That gives a concrete way forward, while also giving a hint for people that want to solve it outside config.php.

Make sure you update all the install docs ;) You can browse around in your mhzgh:patch-2 and click the edit button to update files in this existing PR.

@murrant murrant changed the title Update Installation-Ubuntu-1804-Apache.md Install docs, note proxy config Jan 24, 2019

@mhzgh

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2019

Uhm, didn't even notice the Configuration Docs/Environment pages before to be honest...
Sure, can do.

A note about setting the proxy in config.php to ensure it works during cron or setting the env for cron would be good. You can link to https://docs.librenms.org/Support/Configuration/#proxy-support

Could you give a bit more detail on what you mean by that? Not sure "where" to do modifications now :)

From our discussion I understand that adding a hint nearby the "cron copy" command is not the most optimal location. It should be mentioned in a more general scope nearby config.php.
Since the Install docs hand over that config.php creation part to the WebUI, I am not sure if the Chapter "Web Installer" would be a good place to add further config.php modification hints.

@murrant

This comment has been minimized.

Copy link
Member

commented Jan 24, 2019

Maybe a proxy setting needs to be added to the webui installer?

@mhzgh

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

So you mean e.g. adding an additional step/page in between the "Create user" and "Generate config.php" of the web UI installation wizard, that prompts to configure a http(s) proxy and mentions/refers to the things we discussed?

@murrant

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

@mhzgh don't worry about that, just thinking out loud.

Add a line return before your note and duplicate it to all the install docs and we can merge this.

@mhzgh

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

Oh, ok. Wouldn't feel too confident to extend the web-ui installer without further knowledge :)
Reworked the note a bit to include the ENV or "config.php" references, will update it to the other OS descriptions too

@murrant

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

I like the new text.

You can do it in the same PR. If you want to do it from the github web interface, what I usually do is go to the changes tab, click the edit button (just an easy way to get to the right branch). Then click on the path breadcrumb at the top to go to a different file and click edit on that file.

mhzgh added some commits Feb 13, 2019

@mhzgh

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

Great, thanks for the guidance :)

Changed all the install docs I could find.

@murrant

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

Heh, we usually only update the current install docs... So you were a bit of an over-achiever there.

Thanks :)

@murrant murrant merged commit f7ae5e3 into librenms:master Feb 13, 2019

5 of 6 checks passed

codeclimate Code Climate encountered an error attempting to analyze this pull request.
Details
Inspection Summary
Details
Node: analysis
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details

@lock lock bot locked as resolved and limited conversation to collaborators Apr 14, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.