Remove prepush hooks #130
Remove prepush hooks #130
Conversation
msluther
requested review from
3rd-Eden,
DullReferenceException,
JacopoDaeli,
Swaagie,
asilvas,
bmeck,
burgessa23,
decompil3d,
fritzmonkey,
indexzero,
jcrugzz,
rxmarbles,
samshull,
silasbw and
tarkatronic
samshull
left a comment
samshull
left a comment
There was a problem hiding this comment.
I would like to better understand why removal of this without a replacement is the best choice. Could you clarify for me, please?
| "eslint-plugin-jsx-a11y": "^6.2.3", | ||
| "eslint-plugin-mocha": "^5.3.0", | ||
| "eslint-plugin-react": "^7.14.2", | ||
| "prepush": "^3.1.11", |
There was a problem hiding this comment.
@samshull Random comment location here to allow threaded messages in github.
prepushhas security vulnerability and was being replaced in another PR (linked in this PR description) with a precommit hook.- Why dictate a users's workflow or force them to use
--no-verify. The proper time to verify what someone has done is passing tests is at PR/merge time and not on every commit or push. If a developer wants to commit frequently small chunks of code, that should actually be encouraged. If a developer wants to use TDD and commit an initial set of breaking tests, that should be encouraged/allowed. If a developer wants to push frequently to avoid losing their work, they should be allowed to do so.
There was a problem hiding this comment.
I think doing the way proposed here (no prepush) is more of a "trust but verify" model, which is generally a good thing. You encourage the developer to run the test suite, and trust that they do when necessary. And then when a PR is created, you verify that it passes by running it again.
There was a problem hiding this comment.
There does not appear to be master branch protections on this repo. If we are to rely on PR testing, we should start by changing those rules first. And while I would prefer not to be a devil's advocate here, neither push, nor PR testing will prevent this package from being published to npm with bad code, so I would argue that we should be more concerned with prepublish actions than either of these.
There was a problem hiding this comment.
prepublish sounds like a great idea! Would you be willing to spin up a PR to propose that? :)
There was a problem hiding this comment.
As for branch protection rules, I just got those started with 2 required reviews; once the GH Actions PR gets merged, we can also set those up as required. 👍🏻
There was a problem hiding this comment.
(remember to use prepublishOnly to avoid the silly npm behavior of running prepublish after installs)
samshull
left a comment
samshull
left a comment
There was a problem hiding this comment.
I don't care whether we enforce testing with push hooks or github actions, as long as it accomplishes the goal of ensuring that code in the master branch is passing the tests we have written.
Co-authored-by: Michael Luther <mluther1@godaddy.com>
From this discussion: #129 (comment)