[Wilds] Flesh out most of the new Config objects
ClosedPublic
Actions

Authored by epriestley on Sep 19 2018, 5:36 PM.

Details

Reviewers

amckinley

Maniphest Tasks

T13098: Plans: Arcanist toolsets and extensions

Commits

rARCc64f86c2f632: [Wilds] Flesh out most of the new Config objects

Summary

Ref T13098. This is pretty rough, but sketches out the general shape of a more modern configuration flow. The new flow is very similar to the Phabricator flow.

Each configuration option is typed (string, bool, list of dictionaries, etc) so we can typecheck it, and each type is a class so the types are modular and can do fancy stuff. Some of this "fancy stuff" that I want to do includes:

transparently rewriting/reformatting various options for modernness/consistency;
having some options exposed as objects instead of raw JSON values (in particular, aliases);
merging "list" options (like "aliases") in a modular way instead of by having hard-coded stuff that says "this particular option is magic gets merged instead of getting replaced when defined in multiple places".

Generally, this makes everything modular and extensible and gets rid of the hard-coded switch (...) stuff.

Test Plan

Ran arc get-config, it sort of almost worked.

Diff Detail

Repository

rARC Arcanist

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley created this revision.Sep 19 2018, 5:36 PM

Harbormaster returned this revision to the author for changes because remote builds failed.Sep 19 2018, 5:36 PM

Harbormaster failed remote builds in B20861: Diff 47049!

One thing in here is "String Sources". The idea is that if you have a JSON config file we can expect it to be valid JSON and have the right types, but if you pass --config x=3 we don't have any type information. Marking the source as a string source lets options convert from "3" (what we'll get on the CLI) to 3 (what we'd expect in a config file) and then go through the rest of the pipeline normally.

The ArcanistConfigurationSourceValue object is also a little awkward. It's basically just a <source, value> object. We need to keep track of which source each value came from when merging value lists in case two values aren't mergeable.

An example might be a theoretical config option called animals.ratings which is a dictionary of animals to ratings (cat = 3, mouse = 2), and we'd like to be able to emit this message when merging the different ratings in different config sources:

Configuration (for setting "animals.ratings") is invalid, because two different sources ("Local Configuration File (.arcconfig)" and "User Configuration File (~/.arcconfig)") both specify a value for the key "dog". Each animal rating must be specified in no more than one place in configuration.

This example is somewhat contrived, but it should be possible for configuration to detect and emit this kind of error.

epriestley requested review of this revision.Sep 19 2018, 5:42 PM

Looks good other than minor inlines.

src/config/ArcanistConfigurationSourceList.php
115–117	Why put this in a `try {}` block at all if we're catching `Exception` and then just throwing again right away? Does this have any side effects aside from just making the top level of the call stack consistent?
133	I'm guessing we'll just fix all of these in one go once `arc lint` works again?
src/config/arc/ArcanistArcConfigurationEngineExtension.php
108	For consistency with other documentation, please update this example password to `hunter2`.
122	I just created https://phurl.io/u/commitRanges if you want to put a URL in here.
src/config/option/ArcanistWildConfigOption.php
23–30	I'm not sure if there's more than one copy of this other than `ArcanistScalarConfigOption`, but can this be moved up the class hierarchy?
src/toolset/ArcanistWorkflow.php
65–73	I'm assuming this looks commented-out because of the same spooky syntax highlighting behavior I noticed the other day.
src/toolset/workflow/ArcanistGetConfigWorkflow.php
128–132	We can make this work now just by switching to `$is_verbose` and `echo tsprintf()`, right?

This revision is now accepted and ready to land.Sep 20 2018, 10:42 PM

epriestley marked 7 inline comments as done.Sep 20 2018, 11:17 PM

epriestley added inline comments.

src/config/ArcanistConfigurationSourceList.php
115–117	Currently, this has no effect at all (re-throwing an exception doesn't change its call stack). In the future, I anticipate needing to convert these into some kind of error object/state, save a list of them, and pass them up to the caller. If you have some bogus junk in a config file we likely want to just emit a warning and continue in at least some cases, so you can `arc set-config` your way out of the problem. If we actually `throw` here, you won't be able to fix the issue via `arc set-config` / `arc alias` / whatever. However, none of the validation can really go wrong quite yet so there's no way to actually hit this case or test it. Basically, I'm just kind of marking the blocks that probably need to do something later on, once this code is meaningfully reachable.
133	Yeah, `arc lint` will probably have a lot of improvements to make once it can run again.
src/config/arc/ArcanistArcConfigurationEngineExtension.php
108	haha yes I was completely planning to do this However, I've just removed this completely since it was only used if your entire install required HTTP basic auth for access, which is fairly bizarre and `git blame` suggests affected ~1 install in like 2012. If you do have a configuration like this for some reason, you should be able to whitelist Conduit in your Apache config or use HTTPEngineExtensions now.
122	I was mostly just copying this as previously written, but am considering maybe doing `arc help ranges` something, which has a short refresher and a link to the web ("Read More: ...")? There's currently no way to add arbitrary topics to `help` but it seems like a reasonable sort of thing to do. Maybe this just becomes `arc guide ranges` and `help` keeps its current behavior. Not totally sure yet, but I think we can probably do better than just mentioning the article names.
src/config/option/ArcanistWildConfigOption.php
23–30	This could be moved up, but since `Wild` isn't a `Scalar` I think they'd have to share some base class: `SingleValueConfigOption` `ScalarConfigOption` `StringConfigOption` `BoolConfigOption` `WildConfigOption` `ListConfigOption` `AliasConfigOption` `HostsConfigOption` The `SingleValue` / `Scalar` distinction seemed confusing to me but maybe we'll have more stuff later on which drives adjustment here. This hierarchy wouldn't necessarily be bad, maybe `Scalar` is just not a great name. I'll keep an eye on it as it gets fleshed out more.
src/toolset/ArcanistWorkflow.php
65–73	This one is slightly different: the syntax highlighter is given an entire "reconstructed" file, not a series of snippets. So it sees: /** <...50 blank lines where the context is missing...> return $this->... ...and concludes "oh you have a real big unterminated multi-line comment there". We can't ever get this right in the general case (we'll often be missing the start or end of multiline comments if we're missing context) although I think there's a reasonable argument that we'd get it right more often if we highlighted each block separately. In practice, as long as you use `arc` this isn't really an issue.
src/toolset/workflow/ArcanistGetConfigWorkflow.php
128–132	I think I want to drop "verbose" mode and do this instead to simplify things a bit: `arc get-config` is a summary of everything. `arc get-config x` is a detailed look at config `x`. But I kind of waffled on this and ended up with the flag still existing and this bit commented out. I'd anticipate cleaning it up in the next pass.