Paths

Table of Contentst

allow for subclasses to inject custom config loading in ArcanistConfigurationDrivenLintEngine
AbandonedPublic
Actions

Authored by joshuaspence on May 14 2014, 7:11 PM.

Details

Reviewers

epriestley
NorthIsUp

Group Reviewers

Blessed Reviewers

Maniphest Tasks

Restricted Maniphest Task

Summary

enable configuration overrides for ArcanistConfigurationDrivenLintEngine

Here is a sample subclass that we plan on using at disqus.

<?php

final class DisqusConfigurationDrivenLintEngine extends ArcanistConfigurationDrivenLintEngine {

  public $defaultConfig = '
    {
      "linters": {
        "jshint": {
          "type": "script-and-regex",
          "include": "(\\\\.js$)"
        },
        "text": {
          "type": "text",
          "include": "(\\\\.*)",
          "severity": {}
        },
        "filename": {
          "type": "filename",
          "include": "(.*)"
        },
        "spelling": {
          "type": "spelling",
          "include": "(.*)"
        },
        "flake8": {
          "options": "--ignore=W391,W292,W293,E501,E225,E121,E123,E124,E127,E128,F999,F821 --exclude=*/contrib/*"
        }
      }
    }';


  public function getLintConfigDefaults() {
    return json_decode($this->defaultConfig, true);
  }

  public function getLintConfigOverrides() {
    $working_copy = $this->getWorkingCopy();
    $overrides = $working_copy->getConfig(
      "linters",
      "{}"
      );
    return json_decode($overrides, true);
  }

  public function getLinterConfig() {
    // from .arclint
    try {
      $config = parent::getLinterConfig();
    } catch (Exception $e) {
      $config = array();
    }

    // form this class
    $defaults = $this->getLintConfigDefaults();

    // from .arcconfig
    $overrides = $this->getLintConfigOverrides();

    return array_merge($defaults, $config, $overrides);
  }
}

Test Plan

arc unit still passes?

Diff Detail

Repository

rARC Arcanist

Branch

config

Lint

Lint Warnings

Severity	Location	Code	Message
Warning	src/lint/engine/ArcanistConfigurationDrivenLintEngine.php:3	XHP29	Class Not abstract Or final

Unit

No Test Coverage

Build Status

Buildable 467
Build 1564: [Placeholder Plan] Wait for 30 Seconds
Build 467: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

NorthIsUp updated this revision to Diff 21672.May 14 2014, 7:11 PM

NorthIsUp retitled this revision from to allow for subclasses to inject custom config loading in ArcanistConfigurationDrivenLintEngine.

NorthIsUp updated this object.

NorthIsUp edited the test plan for this revision. (Show Details)

NorthIsUp added a reviewer: epriestley.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptMay 14 2014, 7:11 PM

Herald added subscribers: Korvin, epriestley. · View Herald Transcript

NorthIsUp updated this object.May 14 2014, 7:12 PM

NorthIsUp edited edge metadata.

Harbormaster completed remote builds in B467: Diff 21672.May 14 2014, 7:12 PM

NorthIsUp added inline comments.May 14 2014, 7:12 PM

src/lint/engine/ArcanistConfigurationDrivenLintEngine.php
3	I have no idea what the correct thing to do here is.

NorthIsUp added a subscriber: BYK.May 14 2014, 7:14 PM

joshuaspence added a task: Restricted Maniphest Task.May 14 2014, 8:59 PM

I agree with what you are trying to achieve here (providing some default arclint file that could be reused across projects), but I feel that allowing ArcanistConfigurationDrivenLintEngine to be extended is a step in the wrong direction. The whole point of ArcanistConfigurationDrivenLintEngine is to eliminate the need for distributing custom PHP.

Possibly a more general solution would be to allow user/system level arclint files to be defined to allow defaults to be set.

Yeah, I tend to agree. One attack would be to, e.g., add config to .arcconfig like:

"arclints" : ["/path/to/global", ".arclint", "support/.arclint"]

...and have it load + merge those files or something.

I'd generally like to formally complete T2039 and have it in production for more than, like, a week before building meta-systems on top of it too, though -- there might be things we've gotten wrong that will be much harder to correct after compiled/meta/etc linters.

I also wonder if we should let you put lint configs on Phabricator and have it download them, so you'd specify ruleset names instead of paths. But this rapidly gets messy, and edges us toward arbitrary code execution on users' machines if you compromise Phabricator.

Our use case is to minimize the amount of information that needs to be updated across projects when we change a policy. Having access to a global arclint file is nice, but it still has to be coordinated across projects.

Is there a way to have a global arcconfig that is inherited from?

In D9124#13, @NorthIsUp wrote:

Our use case is to minimize the amount of information that needs to be updated across projects when we change a policy. Having access to a global arclint file is nice, but it still has to be coordinated across projects.

But either method requires coordination between projects right? You either need to distribute the global arclint file to all developers or distribute the PHP class. Or am I missing something here.

I like @epriestley's suggestion of downloading an organisation-wide global arclint file from Phabricator, probably via Conduit (assuming that there is no risk of RCE).

(assuming that there is no risk of RCE).

This seems like a very high bar to meet with "bin", "interpreter", "flags", etc., and doubly so if we apply the same methods to unit tests.

We could prompt the user "there is a new version of this linter config, do you want to trust it? alincoln updated it such and such, here is the diff, yada yada [y/N]" but I think users will blindly "Y" through that 99% of the time and if we also do package management / code distribution stuff we can't reasonably show diffs since they'll often be huge.

In the long run, I think package management is probably what this will end up looking like, but I be happier if it was 2-3 years away and had less user-facing use cases at first (e.g., installing applications in Phabricator).

@joshuaspence You are right, I'm ok with any sort of centrally managed config.

Right now the only option to us is the shared PHP lib, If there was an arcconfig.extends: ['/path/to/global/arc'] that would work too. Either seems realistic in a short time frame.

Ultimately the download from conduit method would be the best I think, but that seems like a ways off.

This would be a long way off, but if I were downloading a file from Phabricator like this, I would want it to have been signed by someone internal to my organization. This would probably involve adding an application to Phabricator for managing GPG keys (or similar) and requiring that the global arclint be signed by some authorised person.

Yeah -- we could probably sign with SSH private keys too, which have the advantage that Phabricator already knows about them and everyone has them, but any kind of signing is going to be a bit of a mess I think.

So, for the short term, what can I do to this diff to get the configuration we need?

In the short term, I guess you can just copy/paste the whole class. That's not ideal, but it probably won't break, and you still get to reuse all the important code (in the linters themselves).

There some possibly-related discussion in T5055.

Generally, I don't see anything reasonable we can do in the short term without opening up a lot of surface area which doesn't really give us a good pathway forward. I want to build toward a long term solution, and I have low confidence that this is a step on that path.

This revision now requires changes to proceed.May 17 2014, 3:39 AM