Fixes T4018. Basically hits the bullet points in that task description except the "ideally" one.
Details
Details
- Reviewers
epriestley - Maniphest Tasks
- T4018: Allow bin/config to affect database configuration and migrate between local and database configuration
- Commits
- Restricted Diffusion Commit
rPc0848bca6d91: Allow bin/config to affect database configuration and migrate between local and…
ran bin/config migrate and saw sensible output.
~> ./bin/config migrate Migrating file-based config to more modern config... Skipping config of source type PhabricatorConfigDatabaseSource... Skipping config of source type PhabricatorConfigLocalSource... Skipping config of source type PhabricatorConfigDefaultSource... Done. Migrated 0 keys.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| src/applications/config/management/PhabricatorConfigManagementGetWorkflow.php | ||
|---|---|---|
| 44 | I'd expect this to break bin/config get during setup. Specifically, on a new install, if mysql.host and similar settings are not configured yet, the user should still be able to use bin/config get to examine other configuration (for example, to review what they've set mysql.host to that isn't working). By jumping directly to the DB config, I think we'll just fail to connect and die here, making bin/config useless during setup. | |
| 91–92 | This is no longer consistent with the source (UI source is "local", actual source is "database"). | |
| src/applications/config/management/PhabricatorConfigManagementMigrateWorkflow.php | ||
| 20–21 | Oh, sorry. I think @hach-que phrased the original task, but the actual use case here is copying out of config files (PhabricatorConfigFileSource), not local config (PhabricatorConfigLocalSource). Specifically, what happened is that users used to have config files (like conf/custom.conf.php). This was hard to figure out so I mostly obsoleted it by using bin/config set for boostrapping into the web UI. However, users with installs from this era (or who have config files for some other reason) don't have an easy way to throw them out. I think this probably needs to look something like:
The desired result is that after running this script, users can destroy their local configuration safely. That means everything present in the file source needs to get migrated somewhere. | |
Comment Actions
The other migration scenario is config values stored in the database, but a future version of Phabricator locks those config values from the web interface. There's currently no way to migrate the DB value to the local config (other than manually deleting it via MySQL and using bin/config set)
Comment Actions
I think this addresses the feedback. ...Except, what should the changes to PhabricatorConfigManagementGetWorkflow end up being? Some sort of flag to get from a different source or? Right now this diff just reverts what would have broken setup, etc.
| src/applications/config/management/PhabricatorConfigManagementGetWorkflow.php | ||
|---|---|---|
| 44 | So should I just revert the changes to this file? | |
Comment Actions
Algorithm looks correct to me. Couple inlines, mostly related to setKeys() / deleteKeys() probably needing implementation.
For get, we should try to include the database, but not explode if we fail. For example:
$ ./bin/config get x { blah blah loacal: "some value" blah blah database: "some other value" }
Or, during startup:
$ ./bin/config get x (Emitted to stderr) Warning: Unable to read database configuration. { blah blah local: "some value" }
| src/applications/config/management/PhabricatorConfigManagementDeleteWorkflow.php | ||
|---|---|---|
| 58–70 | I don't think deleteKeys() works on DatabaseSource (the source isn't isWritable(), so I'd expect this to throw, i.e. config delete --database ... to not work). | |
| src/applications/config/management/PhabricatorConfigManagementMigrateWorkflow.php | ||
| 59 | I'd expect this to fail (not writable). | |
| 62 | String should say "database". | |
| src/applications/config/management/PhabricatorConfigManagementSetWorkflow.php | ||
| 112–124 | As above. | |
Comment Actions
- will use PhabricatorConfigEditor::storeNewValue and introduce some PhabrcatorConfigEditor::deleteConfig
- will fix get as directed
(fixed the string issues already in diffs between your feedback and my initial diff.)
Comment Actions
- introduced PhabrcatorConfigEditor::deleteConfig and used for database deletes
- massaged PhabricatorConfigEditor::storeNewValue to take PhabricatorContentSource + User objects and not just an AphrontRequest
- used PhabricatorConfigEditor::storeNewValue
- made "get" return values for local and database
- detect if empty value returned, and if so say its not set in pertinent config
- wrap database call in try / catch, and if caught, say its cuz db isn't configured properly
Comment Actions
One inline.
| src/applications/config/management/PhabricatorConfigManagementGetWorkflow.php | ||
|---|---|---|
| 46–64 | In theory, emitting JSON from bin/config get allows other things to process it. This emission format isn't machine parseable, since it's impossible to distinguish between an error message and a value which happens to be one of these strings. (And to even make an attempt, a machine parser would have to hard-code these strings.) Consider finding some unambiguous way to report this information. | |
Comment Actions
- add "status" to results of set / unset / error
- add errorInfo which is null unless status is error
- put in errorInfo in misconfigured database case
Comment Actions
linter added trailing commas so also add a return ( something like "),);" looks too gross to me... )