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

Fix hardcoded wwwPath #55

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Conversation

vanosg
Copy link
Collaborator

@vanosg vanosg commented Mar 29, 2015

Issue #42 Check for custom wwwPath and use that instead of default, if it exists. Apply concurrently with patch in brewpi-tools repo

Check for custom wwwPath and use that instead of default, if it exists
@elcojacobs
Copy link
Member

A custom www path is a setting in config.cfg, shouldn't we check the config file if a custom path is set (wwwPath) and get the setting from there?

@vanosg
Copy link
Collaborator Author

vanosg commented Apr 1, 2015

install.sh doesnt modify the config, nor even reference a setting in it- and thats where the user installs to the webPath. I think its very probable the user could install to a non-standard path and not modify the config (ie, the config and actual path may not match)

Does anything else in the code reference the path directly from the config?

I think it is more intuitive for the user if the install script is used to set the path. We could always then write it to the config once the user sets it? Or read the config, see if something is set and prompt the user with that path to install to (but again, I dont think a user would edit the config before running the install script)

Back in #27 I suggested a global settings file to store lot of info like this- while it doesnt necessarily need to be stored in a seperate file, maybe its worth a revisit? Putting these settings into the main config, or a seperate file, may prove useful as the complexity of brewpi increases - and might stop some of these painful variable-passing hacks!

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

Successfully merging this pull request may close these issues.

2 participants