Problem/Motivation

Right now, we have to make sure to run `yarn build` before we commit anything. Especially if we're running with `yarn dev` to do our development. The `build` process is actually minifying CSS, ex.g. so those two do not produce the same output. DrupalCI protects us by rejecting if the build isn't the same as what was pushed, but it would be nice to catch this earlier.

Proposed resolution

We can use a git pre-push hook to replicate the code DrupalCI is doing, which is compiling and checking for a dirty worktree. This keeps it from even getting to DrupalCI, saving Association resources, and developer frustration.

Example pseudo-code from DrupalCI:

yarn run rollup -c
git diff --quiet; NOCHANGES=$?
if [ "$NOCHANGES" -ne 0 ]; then
...
else
...
fi

Remaining tasks

  • ✅ File an issue about this project
  • ☐ Addition/Change/Update/Fix to this project
  • ☐ Testing to ensure no regression
  • ☐ Automated unit/functional testing coverage
  • ☐ Developer Documentation support on feature change/addition
  • ☐ User Guide Documentation support on feature change/addition
  • ☐ Code review from 1 Drupal core team member
  • ☐ Full testing and approval
  • ☐ Credit contributors
  • ☐ Review with the product owner
  • ☐ Release

Issue fork project_browser-3282338

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

chrisfromredfin created an issue. See original summary.

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented

More info here:
https://stackoverflow.com/questions/4196148/git-pre-push-hooks

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Category: Task » Feature request

Dev feature, but still feature

diegors’s picture

diegors
He/Him
Primary language Portuguese, Brazil
Location Campinas - Brazil
commented
Assigned: Unassigned » diegors

Working on it.

diegors’s picture

diegors
He/Him
Primary language Portuguese, Brazil
Location Campinas - Brazil
commented
Assigned: diegors » Unassigned
Status: Active » Needs work

I created the file in .git/hooks/pre-push, with the code:

#!/bin/sh

# colors
RED='\e[31m'
BOLDRED='\033[1m'
NORMAL='\e[0m'

echo "${RED}##################################${NORMAL}"

cd sveltejs; yarn run rollup -c
git diff --quiet; NOCHANGES=$?
if [ "$NOCHANGES" -ne 0 ]; then
echo "";
echo "${RED}##################################${NORMAL}"
echo "${RED}           PUSH ABORTED!!! ${NORMAL}"
echo "${RED}Please, run ${BOLDRED}yarn build${NORMAL}${RED} before push ${NORMAL}"
echo "${RED}################################## ${NORMAL}"
echo "";
exit 1;
fi

I need help on how to commit the hook to the repository, I found a few articles, but they all need to run git config core.hooksPath, or create a script/makefile. Is it possible to commit in a way that works without running anything else?

christyanpaim’s picture

christyanpaim commented
Assigned: Unassigned » christyanpaim
christyanpaim’s picture

christyanpaim commented
Assigned: christyanpaim » Unassigned
chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented

Thanks for this work diegors! I have a couple suggestions:

(1) We include the example hook in the repo or in the contributor.md with instructions on how to set it up.

or

(2) We do something in the package.json "scripts" section to ensure the hook is implemented. That is, we could use something like the "postinstall" git script so that after you run `yarn install` (a necessary step) it copies the hook into the .git/hooks folder, or does the git config for you. ?

Curious for others' thoughts on this. I prefer option 2 because I think it would help to have it running automatically when someone spins up a GitPod instance.

I don't think this would make it into core, but it would be something that would help developers for the time being so could be beneficial.

diegors’s picture

diegors
He/Him
Primary language Portuguese, Brazil
Location Campinas - Brazil
commented
Assigned: Unassigned » diegors

Thanks @chrisfromredfin, I will work on this.

fjgarlin’s picture

fjgarlin commented

We could also leverage GitlabCI so we don't need to worry (locally) about the compiled assets. This could happen at a point in the CI stages and have the files added to the repo. Not sure how doable all of this is, I'm just thinking (writing) out loud.

diegors opened merge request !272

diegors’s picture

diegors
He/Him
Primary language Portuguese, Brazil
Location Campinas - Brazil
commented
Assigned: diegors » Unassigned
Status: Needs work » Needs review

Added post-install script as suggested in #8.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

from the CI:

file git-hooks/pre-push should be 644 not 755

diegors’s picture

diegors
He/Him
Primary language Portuguese, Brazil
Location Campinas - Brazil
commented

Changed git-hooks/pre-push permission to 644.

srishtiiee made their first commit to this issue’s fork.

narendraR made their first commit to this issue’s fork.

narendrar’s picture

narendrar
He/Him
Primary language English
Location Jaipur, India
commented

Not sure if I need to do something extra here, but I run yarn dev and pushed the generated files and it got pushed.

narendrar’s picture

narendrar
He/Him
Primary language English
Location Jaipur, India
commented

git push was aborted when I changed git-hooks/pre-push mode to chmod +x.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Issue tags: +core-post-mvp
diegors’s picture

diegors
He/Him
Primary language Portuguese, Brazil
Location Campinas - Brazil
commented

Did the rebase, MR is mergeable.

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented
Status: Needs review » Reviewed & tested by the community

Love the idea here and I think it will work well / help. We can also later on (separate issue) add in the idea that we can lint before push, etc.

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented
Status: Reviewed & tested by the community » Fixed

There was a lot of consensus / agreement in the PB Slack meeting that this is helpful for DX, so added it!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.