[qa] Add pre-push hook #161

[purhan]

Closes #161

[purhan]

@atb00ker This is functional in the current state, but I would like to add a couple more features:

1. It should be installed automatically (this is a subjective opinion)
2. It requires some tests.

What do you think?

[atb00ker]

It should be installed automatically (this is a subjective opinion)

Can this file be directly added to hooks folder? Do we need to install it for some reason?

Two more things:

1. if script fails because setup for testing is not done, then we should inform the user. 😄
2. Can this file be moved to some other place like .github if adding it to hooks is not possible? 😄

[purhan]

Can this file be directly added to hooks folder? Do we need to install it for some reason?

From what I know, it sits in .git/hooks which is not shared in the working environment. So I'm just creating a symlink into that folder. I looked at other projects who use hooks and this is how some of them did it. Maybe a better solution is there, I will have to do more research.

if script fails because setup for testing is not done, then we should inform the user. 😄

I will try to add this in the next commit

[nemesifier]

I think we should make this reusable for every openwisp module, having this mechanism only in openwisp-utils is not very useful.

If we could make this more generic it would be a lot more useful, eg:

• make it possible to call the script with openwisp-pre-push-hook (similar to openwisp-qa-format and openwisp-qa-check
• runtests.py works only for python modules, we also have runtests and npm packages which get tested with yarn test, would be great if the script is able to figure this out (call the file only if exists, check if package.json is present and in that case use yarn test
• the push may be done without the python virtualenv being enabled, try importing openwisp_utils and if the import fails print to standard error a message that advises to activate the right virtualenv

What do you think @purhan @atb00ker?

[purhan]

@nemesisdesign @atb00ker After the latest commit, openwisp-pre-push-hook can be installed while installing openwisp-utils itself. The hook can then be set up using openwisp-pre-push-hook --install, which can be included in the setup.py script of any module thereafter.
The hook needs to be installed separately as git doesn't allow it for security reasons.

[nemesifier]

Looks good! I think we need some basic automated tests for this new code to ensure it will keep working over time also when new contributors will try to change it.

See also my comment below.

[nemesifier]

here's some more problems, we need to give this a bit more thought if we want it to really be useful.

For some repositories, running all the tests can take a long time, so we usually run tests using --parallel.
But in openwisp-monitoring we can't use --parallel.

More over, in some repos we also run tests with SAMPLE_APP=1.

Dealing with these cases here would complicate things too much.

I think we could just delegate everything to the run-qa-checks script in every repository: this script would also run tests, eg:

• ./run-qa-checks runs ./runtests.py --parallel and SAMPLE_APP=1 ./runtests.py --parallel in openwisp-radius
• ./run-qa-checks runs ./runtests.py and SAMPLE_APP=1 ./runtests.py in openwisp-monitoring
• ./run-qa-checks runs yarn test in openwisp-wifi-login-pages

But we should allow each ./run-qa-checks script to be called from the CI builds, eg:

• ./run-qa-checks --ci, so we can skip the --parallel flag in some cases otherwise the test coverage would drop.

@atb00ker @pandafy @nepython @NoumbissiValere @okraits what do you think? Any other useful suggestion? Or alternative approach?

[atb00ker]

This looks good, @purhan I would suggest before this is merged, open a related PR in another module like controller and use it there for actual testing! 😄

[purhan]

./run-qa-checks --ci, so we can skip the --parallel flag in some cases otherwise the test coverage would drop.

What if we instead, use a flag for the hook (--hook) and everything else works as normal with minimal changes?

[devkapilbansal]

@purhan I would suggest to look in this too Pre-Commit. This can make your work a lot easier

[purhan]

@purhan I would suggest to look in this too Pre-Commit. This can make your work a lot easier

@devkapilbansal this is a handy project but personally I'm not in favor of using third-party libraries for this, though I would still like to hear what the other devs have to say.

[atb00ker]

KISS: Keep it stupid simple.
Libraries are great, but we are doing very small thing here, why add a dependency unless we really need it!? 😄

[purhan]

@atb00ker @nemesisdesign Please take another look at the latest commit. I have moved some of the logic to run-qa-checks which can be changed in any module to work with the hook.

[nemesifier]

I agree on keeping it simple for now and later on as our understanding and needs grow we can decide to switch to a third party library if we deem it worth it.

[coveralls]

Coverage remained the same at 99.757% when pulling d32c30c on Purhan:issues/161-add-pre-push-hook into 30ad216 on openwisp:master.

[atb00ker]

LGTM!
Just please squash the 4 commits! 😄

[purhan]

@atb00ker Done :)

[purhan]

Following up on discussion from openwisp/openwisp-docs#100. Should openwisp-pre-push-hook be renamed and more generalized?

[atb00ker]

I thought this one was already merged? 😮
openwisp-pre-push-hook sounds generic to me! 😄
ping: @nemesisdesign

[nemesifier]

Thanks @atb00ker for pinging me and thanks @purhan for your patience.

Now in order to avoid forgetting about this work and start to use it also in the rest of OpenWISP, what do you suggest would be the next step? Should we add a reference from https://github.com/openwisp/openwisp2-docs?
I guess we already have an issue for that: openwisp/openwisp-docs#100

[purhan]

Now in order to avoid forgetting about this work and start to use it also in the rest of OpenWISP, what do you suggest would be the next step? Should we add a reference from https://github.com/openwisp/openwisp2-docs?
I guess we already have an issue for that: openwisp/openwisp2-docs#100

Yes, I think it belongs in the contribution guidelines. This is more or less an optional feature, could be mentioned in the code conventions section