search

patchew-project / patchew

A patch email tracking and testing system
MIT License
74 stars 25 forks source link

git: option not to update series on tags #143

Open matttbe opened 8 months ago

matttbe commented 8 months ago

Currently, when a new tag is sent on the mailing list, the series is updated in the Git repository.

The push is done with '-o ci.skip', but this seems to be something specific to GitLab. It has no effects with GitHub for example. Not to waste resources on retesting everything and sending notifications once a tag is shared, a new per-project option has been added:

update_series_on_tags

Not to change the current behaviour, series will continue to be updated on new tags by default, except if this option is explicitly disabled.

matttbe commented 8 months ago

Thank you for maintaining this project, and for managing patches for MPTCP.

This modification is mainly for the MPTCP project, because it uses GitHub. New tags trigger CI executions, and some machines are CPU time limited. Maybe other projects have similar limitations.

Note: I was not able to properly test the modification, sorry for that. The modification is quite small and limited to one specific area, is it OK like that?

matttbe commented 8 months ago

Looks reasonable, just had two small comments.

Thank you for the review!

Apart from that, is there no way on GitHub to push without triggering CI?

Yes, there are, but only by modifying the commit message. Both GitHub Actions and Cirrus CI will skip the build if the HEAD commit contains [skip ci]. But they don't use push options like GitLab:

famz commented 8 months ago

Thanks, then I think this solution is okay. It's not ideal to inject [skip ci] in the commit message anyway.

The patch looks good to me now!

matttbe commented 8 months ago

Thanks, then I think this solution is okay. It's not ideal to inject [skip ci] in the commit message anyway.

Indeed. We could add it under a new --- line, so git am would drop it. But I guess that will not please people using the commits directly.

The patch looks good to me now!

Thank you!

Please note that I was still not able to test the modification. I will check if we can modify the MPTCP importer (but I don't control it)

famz commented 8 months ago

For testing, maybe consider adding a case in https://github.com/patchew-project/patchew/blob/master/tests/test_tags.py ?

matttbe commented 8 months ago

For testing, maybe consider adding a case in https://github.com/patchew-project/patchew/blob/master/tests/test_tags.py ?

Thank you! I looked at adding tests in test_git.py, but I had quite a few errors:

test_apply (tests.test_git.GitTest.test_apply) ... ERROR
test_apply_with_base (tests.test_git.GitTest.test_apply_with_base) ... ERROR
test_apply_with_base_and_brackets (tests.test_git.GitTest.test_apply_with_base_and_brackets) ... ERROR
test_git_push_options (tests.test_git.GitTest.test_git_push_options) ... ok
test_need_apply (tests.test_git.GitTest.test_need_apply) ... ok
test_need_apply_incomplete (tests.test_git.GitTest.test_need_apply_incomplete) ... ok
test_need_apply_multiple (tests.test_git.GitTest.test_need_apply_multiple) ... ok
test_rest_apply_failure (tests.test_git.GitTest.test_rest_apply_failure) ... ok
test_rest_apply_success (tests.test_git.GitTest.test_rest_apply_success) ... FAIL
test_rest_git_base (tests.test_git.GitTest.test_rest_git_base) ... ERROR
test_rest_need_apply (tests.test_git.GitTest.test_rest_need_apply) ... ok
test_rest_unapplied (tests.test_git.GitTest.test_rest_unapplied) ... ok
test_rest_unapplied_target_repo (tests.test_git.GitTest.test_rest_unapplied_target_repo) ... ok
test_result_data_automatic_url (tests.test_git.GitTest.test_result_data_automatic_url) ... ok

I used the same commands as the ones used by GitHub Actions, but with Python 3.11:

python3 -m venv ./venv
. venv/bin/activate
pip install -r requirements.txt
python manage.py test -v 2

I don't know if these tests are supposed to fail.

But when looking closer now, I notice test_git_push_options is passing, and I need something similar. I can check that.

matttbe commented 8 months ago

@famz thank you for the suggestion. I just added a new test validating the new feature. I don't know well the environment, I guess it is possible to do better. But I checked with and without the option, and I have the expected behaviour.

matttbe commented 8 months ago

For testing, maybe consider adding a case in https://github.com/patchew-project/patchew/blob/master/tests/test_tags.py ?

Thank you! I looked at adding tests in test_git.py, but I had quite a few errors:

I just took a bit of time to understand the issue: it was due to a git config option:

git config --global push.gpgsign true

Without that, tests are OK, I should have checked the tests before, sorry for the noise :)