[FIX] server_environment: Protect import - #670
Conversation
|
Re #667 |
|
Hey @pedrobaeza, The Thanks! |
tedsalmon
left a comment
There was a problem hiding this comment.
Made some suggestions based on my previous comment :)
| ) | ||
| if not system_base_config.get('running_env', False): | ||
| raise Exception( | ||
| "The parameter 'running_env' has not be set neither in base " |
There was a problem hiding this comment.
Though I know you didn't write this exception, It could use a little sprucing up. Might I suggest:
"The parameter 'running_env' is not set in the base config file, "
"option -c or in openerprc.\n"
"We strongly discourage the use of an rc file and instead "
"recommend you explicitly add the following options into your "
"config:\n[options]\nrunning_env = [dev|prod]"
| "please add a folder %s" % ck_path | ||
| ) | ||
| if not system_base_config.get('running_env', False): | ||
| raise Exception( |
There was a problem hiding this comment.
EnvironmentError makes more sense than a plain Exception
| ck_path = os.path.join(_dir, system_base_config['running_env']) | ||
|
|
||
| if not os.path.exists(ck_path): | ||
| raise Exception( |
There was a problem hiding this comment.
EnvironmentError makes more sense than a plain Exception
| _boolean_states = {'1': True, 'yes': True, 'true': True, 'on': True, | ||
| '0': False, 'no': False, 'false': False, 'off': False} | ||
|
|
||
| if not system_base_config.get('running_env', False): |
There was a problem hiding this comment.
Why not move this logic (L39:L54) into _load_config() and then wrap the serv_config = _load_config() in a try/except for EnvironmentError?
|
@t3ddftw, thanks for the suggestion, but as this is only a PR for fixing the error on branches with this module not installed, is out of bounds to refactor it, so I prefer to keep it as is, because I'm not even using it. P.S.: cryptic login you have, hehe |
|
OK, fast-tracking as stated. Please make another PR for the suggestion you made. |
|
Yeah, we aren't using this either but similarly to you, we ran into issues deploying this repo. I'll submit another PR as you suggested as I don't like the fact that you can still run into an Haha yes, this has been my GitHub login for too long to give it up now. Thanks! |
Syncing from upstream OCA/server-tools (9.0)
Trying to fix the mess that this module merge has provoked.
Fast-tracking if all goes right.