✨ 🏗 Add a script to enable a pre-push hook that tests local changes before `git push` #15507

rsimha · 2018-05-23T00:48:16Z

This PR does the following:

Adds a default pre-push hook for AMP at build-system/default-pre-push, which runs:
- gulp bundle-size (if dist/v0.js was built) (~2 sec)
- gulp lint --local-changes (~3 sec)
- gulp test --local-changes (~5 sec)
Adds a script called build-system/enable-git-pre-push.sh, which when run, will install a shortcut to build-system/default-pre-push at .git/hooks/pre-push
Once enabled, whenever a developer runs git push, the pre-push hook will first run, and if it fails, the push is canceled
Makes minor updates to gulp test --local-changes and gulp bundle-size so they work well as pre-push hooks

Usage:

To opt in, simply run ./build-system/enable-git-pre-push.sh from the root directory.
Every call to git push will first run .git/hooks/pre-push
The total time to run all pre-push hooks is ~10 seconds
To skip the hook, run git push --no-verify
To remove the hook, simply delete .git/hooks/pre-push

Note: We use a level of indirection instead of copying build-system/default-pre-push to .git/hooks/pre-push so that the default behavior can be changed at a later time, and developers who have opted in don't have to reinstall the hook to get the new behavior.

Fixes #15369

rsimha · 2018-05-23T00:48:28Z

/to @kristoferbaxter @choumx

kristoferbaxter · 2018-05-23T15:46:46Z

Might be a good idea to include the directions to enable these push hooks within the contributing guides.

kristoferbaxter

Left a few questions.

kristoferbaxter · 2018-05-23T15:47:54Z

build-system/default-pre-push

+eval $GULP_TEST_LOCAL || exit 1
+echo -e "\n"
+
+echo $(GREEN "Done with") $(CYAN "pre-push") $(GREEN "hooks. Pushing commits to GitHub...")


I believe this hook will run even when pushing to other locations than Github. Is there something I'm missing?

The script gets installed (via a shortcut) to ~src/amphtml/.git/hooks/pre-push. Doesn't this mean it gets run only when you push to ampproject/amphtml?

The remote isn't necessarily on Github. I'd call it good enough though unless it's confusing to others.

kristoferbaxter · 2018-05-23T15:49:00Z

build-system/enable-git-pre-push.sh

+echo $(GREEN "  3. With this,")  $(CYAN "git push") $(GREEN "will first run the checks in") $(CYAN "$PRE_PUSH_SRC")
+echo $(GREEN "  4. You can edit") $(CYAN "$PRE_PUSH_DEST") $(GREEN "to change the pre-push hooks that are run before") $(CYAN "git push")
+echo $(GREEN "  5. To skip the hook, run") $(CYAN "git push --no-verify") $(GREEN "or") $(CYAN "git push -n")
+echo $(GREEN "  6. To remove the hook, delete the file") $(CYAN "$PRE_PUSH_DEST")


Thanks for adding this description to the installation script. This should reduce the number of questions about the hook.

kristoferbaxter · 2018-05-23T15:49:40Z

build-system/default-pre-push

+# before running "git push".
+#
+# Default pre-push hook for AMPHTML. To enable, run:
+#     "./build-system/enable-git-pre-push.sh"


Out of curiosity, what the plan for Windows developers?

I believe Windows users install git in order to develop AMP, and therefore, should already be able to run scripts such as this one. I'd be willing to try this if you have a Windows machine. In any case, since this isn't a required part of the development flow, we can always update it at a later time.

Perhaps leave a comment or something similar about Windows development.

Will do this when I add it to the Dev docs. Shouldn't be long before we know if this works or not!

rsimha

I wanted to get a few people to try this for a while and see if it's an effective guard, before announcing it broadly via our dev docs. Let me know if that sounds good to you.

rsimha · 2018-05-23T18:39:35Z

build-system/default-pre-push

+eval $GULP_TEST_LOCAL || exit 1
+echo -e "\n"
+
+echo $(GREEN "Done with") $(CYAN "pre-push") $(GREEN "hooks. Pushing commits to GitHub...")


The script gets installed (via a shortcut) to ~src/amphtml/.git/hooks/pre-push. Doesn't this mean it gets run only when you push to ampproject/amphtml?

rsimha · 2018-05-23T18:41:09Z

build-system/default-pre-push

+# before running "git push".
+#
+# Default pre-push hook for AMPHTML. To enable, run:
+#     "./build-system/enable-git-pre-push.sh"


I believe Windows users install git in order to develop AMP, and therefore, should already be able to run scripts such as this one. I'd be willing to try this if you have a Windows machine. In any case, since this isn't a required part of the development flow, we can always update it at a later time.

kristoferbaxter · 2018-05-23T19:05:55Z

Waiting to add it to dev docs makes complete sense until we're certain it works as intended. Good idea!

rsimha · 2018-05-23T19:20:37Z

@kristoferbaxter This is now merged.

Step 1: Run ./build-system/enable-git-pre-push.sh
Step 2: Let me know if this works for you

:)

kristoferbaxter · 2018-05-23T19:37:20Z

./build-system/enable-git-pre-push.sh: line 23: realpath: command not found

kristoferbaxter · 2018-05-23T19:40:37Z

Linting error did fail the push.

rsimha · 2018-05-23T20:27:26Z

@kristoferbaxter I've ~~sent out~~ merged #15535 to fix the realpath issue.

rsimha added the WG: infra label May 23, 2018

rsimha self-assigned this May 23, 2018

rsimha requested review from kristoferbaxter and dreamofabear May 23, 2018 00:48

googlebot added the cla: yes label May 23, 2018

Add a hook that tests local changes before git push

bc169ab

kristoferbaxter reviewed May 23, 2018

View reviewed changes

rsimha commented May 23, 2018

View reviewed changes

kristoferbaxter approved these changes May 23, 2018

View reviewed changes

rsimha merged commit 13c7181 into ampproject:master May 23, 2018

rsimha deleted the 2018-05-21-PrePush branch May 23, 2018 19:17

rsimha mentioned this pull request May 23, 2018

🐛 Don't use realpath in shell scripts #15535

Merged

This was referenced May 24, 2018

🐛 Clean up pre-push messages #15547

Merged

✨🏗 Enhance gulp test--local-changes to detect and run unit tests related to a set of changes #15585

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ 🏗 Add a script to enable a pre-push hook that tests local changes before `git push` #15507

✨ 🏗 Add a script to enable a pre-push hook that tests local changes before `git push` #15507

rsimha commented May 23, 2018 •

edited

rsimha commented May 23, 2018

kristoferbaxter commented May 23, 2018

kristoferbaxter left a comment

kristoferbaxter May 23, 2018

rsimha May 23, 2018

kristoferbaxter May 23, 2018

kristoferbaxter May 23, 2018

kristoferbaxter May 23, 2018

rsimha May 23, 2018

kristoferbaxter May 23, 2018

rsimha May 23, 2018

rsimha left a comment

rsimha May 23, 2018

rsimha May 23, 2018

kristoferbaxter commented May 23, 2018

rsimha commented May 23, 2018

kristoferbaxter commented May 23, 2018

kristoferbaxter commented May 23, 2018

rsimha commented May 23, 2018 •

edited

✨ 🏗 Add a script to enable a pre-push hook that tests local changes before git push #15507

✨ 🏗 Add a script to enable a pre-push hook that tests local changes before git push #15507

Conversation

rsimha commented May 23, 2018 • edited

rsimha commented May 23, 2018

kristoferbaxter commented May 23, 2018

kristoferbaxter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsimha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristoferbaxter commented May 23, 2018

rsimha commented May 23, 2018

kristoferbaxter commented May 23, 2018

kristoferbaxter commented May 23, 2018

rsimha commented May 23, 2018 • edited

✨ 🏗 Add a script to enable a pre-push hook that tests local changes before `git push` #15507

✨ 🏗 Add a script to enable a pre-push hook that tests local changes before `git push` #15507

rsimha commented May 23, 2018 •

edited

rsimha commented May 23, 2018 •

edited