Page MenuHomePhabricator

Adds ConfigurationLocalSource class

Authored by chris on Jul 28 2019, 12:27 PM.



This adds an additional class for reading and writing local configuration values.
My rough plan here is to use this for some initial configuration and setup scripts/checks
so that we can walk users through installation and configuration a little more reasonably.
(Like, check that database credentials are set and the database is reachable; set a base
URI; etc.)

Immediate next step will probably be to build that setup user flow and a utility to allow
users to programmatically interact with configuration sources, e.g., via

${app_root}/bin/config set

or similar

Test Plan

Just code review for now. Verified that all the JSON read/write ops work
as expected

Diff Detail

rSIM Simplex
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

chris requested review of this revision.Jul 28 2019, 12:27 PM
chris created this revision.
chris updated this revision to Diff 44.Jul 28 2019, 12:36 PM

Unbreak some imports

chris updated this revision to Diff 45.Jul 28 2019, 12:59 PM

Just go ham on the imports

chris added a comment.Jul 28 2019, 1:00 PM

@brooke --- I kind of went to town on imports in this diff. It's all just cosmetic stuff, so go ahead and ignore that when looking at the SimplexConfigSource stuff

brooke added inline comments.Jul 28 2019, 7:51 PM

why do you need typing here?


there is no 'if statement' or 'try'. Under what situation the exception will be raised?


why is it not self.config = self.load_config()?
self.workflow = management.SimplexManagementWorkflow()?


here and line 37, do we want to do f.close()?


here and line 63, return nothing?

chris added inline comments.Jul 28 2019, 8:16 PM

Good catch! Originally I was going to have a more complicated interaction between the config sources and simplex env classes, and was planning to use Typing to say "You must supply an instance of class SimplexConfigSource or any subclass of it" to some function call or other. But then I realized that was more complicated than it needed to be and nixed the idea.

I'll remove this import!


Just programming defensively here. Basically just explicitly treating all config sources as read-only unless that rule is overridden in a subclass definition.


Cause I'm a dumb-dumb.

Apparently I fixed this in D16 though, so I'm just going to ignore it here


The with file handler takes care of that for us automatically, actually!


Yup! Either it works and the function returns, or it fails and errors loudly. We don't really care about what the output of a set operation is, I don't think?

brooke accepted this revision.Aug 4 2019, 1:19 PM
This revision is now accepted and ready to land.Aug 4 2019, 1:19 PM
This revision was automatically updated to reflect the committed changes.