Hi Armand, > I personally agree and this is feasible, please create a ticket regarding that request and assign it to me
Just to be clear, I didn't want to imply that you are somehow responsible for this, even before the recent change our commit hook was doing basically the same thing with pip/pylint. I just had to rebase several worktrees at once over the commit which changed the virtualenv definition, which made it very noticable. However, I appreciate your enthusiasm, so I went ahead and complied with the request ;) https://issues.apache.org/jira/browse/MESOS-8217 On Fri, Nov 10, 2017 at 8:47 AM, Armand Grillet <agril...@mesosphere.io> wrote: > Hi Benno, > > As I added the new JavaScript linter to support/mesos-style.py, let me > provide some details regarding its implementation. > > "Indeed, it turns out that the pre-commit hook is installing both a pip > package and an npm package, eslint, along with all of its 1450 > dependencies. (https://pastebin.com/hTZWRxcy)" > > We chose ESLint due to its flexible CLI and good documentation, it provides > us an output going well with the two other linters we use for C++ and > Python and a linting that we have adapted to work with our JS codebase. > Concerning the metric you used: JSHint, which is the other JS linter > allowing customization from the collection > https://github.com/collections/clean-code-linters, has 972 dependencies > (number obtained using npm-remote-ls); this is a smaller number but still > in the same league. There is no equivalent to cpplint.py for JS, we had to > use a Node.js package (in that case a npm package). > > Node.js packages require node, we install it from a new Python virtual > environment created for the linters (the Mesos Python linter also requires > some pip packages) using nodeenv (https://github.com/ekalinin/nodeenv). > This was a simple solution to install eslint and it explains the chain of > dependencies you described (pip package => npm package => JS dependencies). > > "My proposal would be to move most checks to post-reviews.py, which > is a slow operation anyways and which actually marks the point in time > where the commits should be cleaned up enough to pass all checks." > > I personally agree and this is feasible, please create a ticket regarding > that request and assign it to me. Comments from other devs would be useful > to know if this would be a positive change or not. > > One last point regarding the length of the installation of this new linter > due to its dependencies: the two Python virtual environments we use in > Apache Mesos (one for the CLI, one for the linters) are only being rebuilt > when we update the pip requirements or update the virtual environment > installation scripts ( > https://github.com/apache/mesos/blob/ab9039335723fc36cb8e8b4fbb17a1 > 5327520cc9/support/mesos-style.py#L406). > This is a not a common event as you can see from the history of the CLI > pip-requirements.txt: > https://github.com/apache/mesos/commits/master/src/python/cli_new/pip- > requirements.txt > and > https://github.com/apache/mesos/commits/2d19111e4852aed25161e4549ff704 > f9d4c2f37b/src/cli_new/pip-requirements.txt > . > > Best regards, > > 2017-11-10 16:28 GMT+01:00 Benno Evers <bev...@mesosphere.com>: > > > Hi guys, > > > > Our commit hooks in mesos have been traditionally quite strict (something > > many people here will have experienced when "quickly" commiting some work > > in progress before changing branches). However, when I rebased a branch > > today I was surprised to learn that I had to wait more than 5 minutes > while > > something was downloaded from the internet. > > > > Indeed, it turns out that the pre-commit hook is installing both a pip > > package and an npm package, eslint, along with all of its 1450 > > dependencies. (https://pastebin.com/hTZWRxcy) > > > > Aside from the security implications (these are unsigned, and every > single > > one gets to execute custom javascript) I feel like there is something > > fundamentally wrong here - a git commit is supposed to be a local > > operation, it should not need internet access: > > > > bevers@poincare:~/mesos$ git commit -m "Dummy commit on cleaned > > worktree." > > Virtualenv not detected... building > > Rebuilding virtualenv... > > Retrying (Retry(total=4, connect=None, read=None, redirect=None)) > after > > connection broken by > > 'NewConnectionError('<pip._vendor.requests.packages.urllib3.connection. > > VerifiedHTTPSConnection > > object at 0x7fea82eb11d0>: Failed to establish a new connection: [Errno > -2] > > Name or service not known',)': /simple/pip/ > > [...] > > Retrying (Retry(total=0, connect=None, read=None, redirect=None)) > after > > connection broken by > > 'NewConnectionError('<pip._vendor.requests.packages.urllib3.connection. > > VerifiedHTTPSConnection > > object at 0x7f2e627456d0>: Failed to establish a new connection: [Errno > -2] > > Name or service not known',)': /simple/nodeenv/ > > Could not find a version that satisfies the requirement > nodeenv==1.1.2 > > (from -r /home/bevers/mesos/support/pip-requirements.txt (line 1)) (from > > versions: ) > > No matching distribution found for nodeenv==1.1.2 (from -r > > /home/bevers/mesos/support/pip-requirements.txt (line 1)) > > > > > > I'm sure running various linters improves code quality, but there must > be a > > better way to implement this. My proposal would be to move most checks to > > post-reviews.py, which is a slow operation anyways and which actually > marks > > the point in time where the commits should be cleaned up enough to pass > all > > checks. > > > > What do you think? > > > > Best regards, > > -- > > Benno Evers > > Software Engineer, Mesosphere > > > > > > -- > Armand Grillet > Software Engineer, Mesosphere > -- Benno Evers Software Engineer, Mesosphere
