search

plone / bobtemplates.plone

Python Code Templates for Plone Projects with mr.bob
https://pypi.org/project/bobtemplates.plone/
GNU General Public License v2.0
24 stars 31 forks source link

bobtemplates plone breaks plone.recipe.codeanalysis pre-commit hook #184

Closed tisto closed 7 years ago

tisto commented 7 years ago

Since we removed the travis.cfg and set "return-status-codes = True" in our buildout.cfg the p.r.codeanalysis pre-commit returns a status code 1. This prevents a commit to be executed if there is a code violation. This is extremely annoying for developers. It was a conscious design decision in p.r.codeanalysis to not prevent the commit but just notify the developer.

I would propose to either re-add a travis.cfg (quick fix) or find a better way to work around this (also fine with me but I don't have time to do it).

cc @hvelarde

hvelarde commented 7 years ago

have you tried git commit --no-verify or simple git commit -n? this option bypasses the pre-commit and commit-msg hooks:

https://git-scm.com/docs/git-commit#git-commit--n

if you want just to notify you must use return-status-codes = False, that runs the pre-commit hook but don't fail on issues.

tisto commented 7 years ago

@hvelarde I wrote that pre-commit hook. I know how it works and how I can by-pass it. The issue is that bobtemplate.plone uses plone.recipe.codeanalysis in a wrong way as described above since we removed travis.cfg. To fix this we either have to re-add travis.cfg or change the way p.r.codeanalysis does the pre-commit hook.

hvelarde commented 7 years ago

@tisto I think I don't understand your issue; can you explain again what do you want and why the current configuration doesn’t work for you?

tisto commented 7 years ago

@hvelarde sure. you can configure p.r.codeanalysis to return status codes or not. If you return status codes, you break the pre-commit hook. If you have a violation, the pre-commit hook that runs bin/code-analysis returns status code 1. This prevents the git commit to be executed at all without any error message.

Therefore it is necessary to set return status code to 0 on your development machine (the default value for p.r.codeanalysis).

On travis, you need those status codes though, because there is no other way to fail the build if you have a violation. Therefore we need a travis.cfg to reflect that difference.

Proper CI systems (Jenkins) have a way to handle code violations in a better way. Travis does not offer that unfortunately. Changing the way p.r.codeanalysis works because we want to use it with a limited CI system does not seem to be right...

hvelarde commented 7 years ago

the travis.cfg was not meant to deal with code analysis stuff in the first place; it was an artefact created by @datakurre to speed up builds when Travis CI had serious restrictions on build timeouts and no support for containers and caching.

can you list the error messages that are annoying your developers? because, if you are talking about the TODO, or some of the plone.api ones, I think the problem is different: some flake8 plugins are returning errors when they should return warnings; that's a known issue and I have reported it to @gforcada in the past.

tisto commented 7 years ago

@hvelarde travis.cfg was created by asko but I used it for p.r.codeanalysis as well.

This has nothing to do with the flake-8 plugins. I designed the system like this on purpose. It might be hard to understand, but there is no way around re-adding travis.cfg. We are currently using p.r.codeanalysis in a wrong way, period. bobtemplates plone should reflect best practices in the Plone community, not bad practices. The other option is to drop code analysis alltogether.

hvelarde commented 7 years ago

you're not providing enough information; what's the "right" way to use plone.recipe.codeanalysis? what are the best practices we are not following?

you can change the configuration of any package to work the way you want, but trying to force everybody else to use it the way you want without explaining why you want it, is not the way to go.

so, please provide additional information; you may be right, but I keep not understanding you.

mauritsvanrees commented 7 years ago

In the bobtemplates.plone repo we have a travis.cfg to test the bobtemplates.plone package itself. That file has return-status-codes = True, which overrides the setting in our buildout.cfg which uses the default.

When you use bobtemplates.plone to create a package, then this new package has no travis.cfg but it has a buildout.cfg which has return-status-codes = True.

So within bobtemplates.plone we are following two practices.

Since in plone.app.codeanalysis the default is False, it seems reasonable to follow that. Easiest would be:

That would mean partially reverting pull request #166.

tisto commented 7 years ago

@hvelarde please create a new package with the latest bobtemplates.plone, build it, add a code violation and then try to commit something. You will see the code-violations are reported, you don't see that anything bad could have happened. If you check your git log you will see that no commit has taken place (without any notification to the user that the command she executed was dropped). This is what we have right now and certainly not the behavior anybody could want.

Look at the p.r.codeanalysis README, what it says about the return-status-code option:

https://github.com/plone/plone.recipe.codeanalysis/blob/master/README.rst

"If set to True, the bin/code-analysis script returns an error code that Continuous Integration servers (like Travis CI) can use to fail or pass a job, based on the code analysis output..."

In the first draft this setting was even called "travis = true". We are currently using the setting in a way that was never intended by the author of that package (me). Using the pre-commit-hook setting together with return-status-code just does not work and was never intended to be used together. Why would anybody want to use a pre-commit on travis?

I fully agree with @mauritsvanrees. Those are exactly the steps we need to take.

hvelarde commented 7 years ago

@tisto I'm totally fine with what you want to do; you are the ones using this generator, not me.

the main reason I worked on the above mentioned PR was to get rid of an old-fashioned and outdated way of implementing caching on packages for Travis CI; nobody knew at that time that that would cause some unintended consequences in code analysis for you, because the way you work.

resuming, this is what I think:

I don't agree with your rationale but, as I said above, that's not the point; please feel free to put the travis.cfg file back if you're in a hurry, as I'm currently trying to finish a report for a customer.

if you want me to fix it you'll have to wait until I finish that.

hvelarde commented 7 years ago

@tisto I was reading the Git documentation and I think there's no problem with plone.recipe.codeanalysis:

The pre-commit hook is run first, before you even type in a commit message. It’s used to inspect the snapshot that’s about to be committed, to see if you’ve forgotten something, to make sure tests run, or to examine whatever you need to inspect in the code. Exiting non-zero from this hook aborts the commit, although you can bypass it with git commit --no-verify. You can do things like check for code style (run lint or something equivalent), check for trailing whitespace (the default hook does exactly this), or check for appropriate documentation on new methods.

so, just to be clear, is this what you want?

that can easily be solved using a command line option on buildout running in CI:

bin/buildout code-analysis:return-status-codes=True

so, there's no need to have a travis.cfg file, neither.

tisto commented 7 years ago

@hvelarde proper CI systems will not fail the job on code violations but just mark a build as yellow (between green and red). Devs are supposed to see violations in the pre-commit hook and then fix them with "--amend". This workflow worked for me and my teams quiet well so far and I don't see any good reason to change that.

If you think the p.r.codeanalysis README is "wrong", feel free to do a PR. It may be obvious to you, it is not obvious to me.

hvelarde commented 7 years ago

@tisto the README file is not clear in this part:

return-status-codes If set to True, the bin/code-analysis script returns an error code that Continuous Integration servers (like Travis CI) can use to fail or pass a job, based on the code analysis output. Note that Jenkins usually does not need this option (this is better handled by the Jenkins Violations plugin). Note that this option does not have any effect on the other code analysis scripts. Default is False.

as this directive also affects how the pre-commit hook works: if we don't return the status code, the pre-commit hook will not abort the commit on failures.

I hope this make it clear for you also.

tisto commented 7 years ago

This issue was not solved by #185. We just effectively removed running code-analysis on travis alltogether.

tisto commented 7 years ago

@hvelarde it is really frustrating for me to see how much time I have to invest into reverting every single change that you proposed in this package...I invested a lot of time into creating a sound testing and ci story for this package and I have to fight over and over again to keep the features that we already had working. Maybe it is time for me to stop contributing to this package alltogether...

hvelarde commented 7 years ago

@tisto just a friendly reminder that this is not your package and that changes proposed were reviewed and merged by other members of the community; why you didn't say nothing back then? if you had taken your time then we both have saved frustration and time, because I'm also wasting mine here.

exactly what features are lost? include a list so we can discuss and fix them.

explain yourself well and stop threatening with leaving as everybody does, that's not helping.

mauritsvanrees commented 7 years ago

We just effectively removed running code-analysis on travis alltogether.

I don't see this. The generated .travis.yml has this:

install:
...
  - bin/buildout -N buildout:download-cache=downloads code-analysis:return-status-codes=True
...
script:
  - bin/code-analysis
  - bin/test

So that seems good.

The .travis.yml of bobtemplates.plone itself also runs bin/code-analysis

There are differences between the way in which the package and the generated package implement this, but at first glance it seems okay.

tisto commented 7 years ago

@mauritsvanrees code-analysis runs but it will not return a status code. So it will always pass. As said many times before. We need a travis.cfg that sets "return-status-code = True".

hvelarde commented 7 years ago

@tisto code-analysis runs an should return a status code without needing an additional travis.cfg file:

https://github.com/plone/bobtemplates.plone/blob/master/bobtemplates/plone_addon/.travis.yml.bob#L13-L14

hvelarde commented 7 years ago

@tisto I created a test case here and now I think I understand the problem:

https://github.com/plone/bobtemplates.plone/tree/hvelarde-testcase

Travis is not failing because it's not running the code-analysis script included in the generated package:

https://travis-ci.org/plone/bobtemplates.plone/jobs/219727510#L2198

I'll take a look on this later.