search

tobiipro / support-firecloud

Software and configuration that support TobiiPro's Cloud Services development.
Apache License 2.0
4 stars 1 forks source link

run 'make check' before 'git push' #109

Closed andreineculau closed 5 years ago

andreineculau commented 5 years ago

goal is to have a quick feedback cycle, as opposed to pushing code that will trigger a CI agent to spin up, bootstrap itself, fetch dependencies, build your code and only then fail with "expected 2 spaces, not 3"

it is a pre-push hook, as opposed to a pre-commit because

  1. you should be allowed to commit "wip" code, rewrite history to clean it up, etc
  2. make check is fast, but not that fast, so depending on the size of the repo, you might be looking at ~1 minute waiting time (atex-platform and gs-server have ~40 seconds)

I ran it with positive outcomes for a couple of weeks myself

NOTE that the PR in its current form wants to make this the default with a opt-out (potentially for repos like atex-platform and gs-server). The alternative would be to make it a opt-in, and then have each repo add a line to their root Makefiles that includes this inc.mk file. The threshold of how much is too much is subjective, but I can only guess that it is a good-to-have for most of the repos thus the proposition to have it on by default, and off for only those "monolithic" repos.

// @fredrikj @jonashogstrom you were talking of doing the same, thus your feedback is appreciated

jonashogstrom commented 5 years ago

will it run make check on the local copy of the source? So it might fail if I for some reason have WiP code that doesnt pass even though it is not committed yet? I guess as long as you are aware of this it should be fine. +1 from me.

andreineculau commented 5 years ago

will it run make check on the local copy of the source?

:+1: good that you bring that up! the answer is both yes and no.

no because we actually make check looks at "cached" files i.e. files that are committed or staged.

yes because if you stage some lines, but not all, we will check the whole file

my take is that partial staging is not something popular (at least not in this circle). but I can add a comment (and maybe even a warning that some files are partially staged)

andreineculau commented 5 years ago

PR updated with some visual info

andreineculau commented 5 years ago

@jonashogstrom done (pre-commit -> pre-push)

andreineculau commented 5 years ago

merged, I will update a few repos in atex/drive/controller with this change, before leaking into all repos