search

python / cherry-picker

🐍🍒⛏ Utility script for backporting/cherry-picking CPython changes from master into one of the maintenance branches.
Apache License 2.0
49 stars 39 forks source link

Clarify that ``git`` ``2.28.0+`` is needed for tests #66

Closed DanielNoord closed 2 years ago

DanielNoord commented 2 years ago

git init --initial-branch was only added in 2.28. It's being used in one of the fixtures so this should probably be documented.

See: https://git-scm.com/docs/git-init/2.28.0

DanielNoord commented 2 years ago

Documenting this in the readme sounds good, however it might be useful to add a skip to the fixture too, so that:

A skip or a warning? With a skip they would always skip 10+ test.

  1. some of the tests that don't rely on this option will still work.

pytest automatically runs those still I believe. For me the test suite only broke on about 10/15 tests. The rest passed.

ezio-melotti commented 2 years ago

Both work, as long as there is some indication that the tests are failing because of an old version of git.

DanielNoord commented 2 years ago

Does anybody know what this linting error is? I saw a lot of subprocess calls where the git command is set up in a string before the actual call. I thought this was because of line lengths, but I'm now thinking that this is a way to avoid this linting warning. Is that correct? And if so, doesn't it make sense to just turn it off?

hugovk commented 2 years ago

It's because the git command is being found in PATH instead of being "fully qualified relative to the filesystem root".

https://bandit.readthedocs.io/en/1.7.4/plugins/b607_start_process_with_partial_path.html

Perhaps this can be skipped because it's in a test and not production code?

DanielNoord commented 2 years ago

It's because the git command is being found in PATH instead of being "fully qualified relative to the filesystem root".

https://bandit.readthedocs.io/en/1.7.4/plugins/b607_start_process_with_partial_path.html

Perhaps this can be skipped because it's in a test and not production code?

The current trick of just initialising a variable and using the variable doesn't really solve the issue that bandit is warning for though, right? Might as well disable it for this file.

hugovk commented 2 years ago

I don't know, you can try it!

Rather than skipping the whole file, we can skip just this check for just this line with # nosec B607

DanielNoord commented 2 years ago

I don't know, you can try it!

Rather than skipping the whole file, we can skip just this check for just this line with # nosec B607

Please see https://github.com/python/cherry-picker/pull/71/.

Passes bandit locally for me!

DanielNoord commented 2 years ago

CI fixed 😄