Skip to content

git: option not to update series on tags #143

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matttbe
Copy link

@matttbe matttbe commented Feb 14, 2024

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 matttbe marked this pull request as ready for review February 14, 2024 15:44
@matttbe
Copy link
Author

matttbe commented Feb 14, 2024

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?

Copy link
Collaborator

@famz famz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable, just had two small comments.

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

@matttbe
Copy link
Author

matttbe commented Feb 16, 2024

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
Copy link
Collaborator

famz commented Feb 16, 2024

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
Copy link
Author

matttbe commented Feb 16, 2024

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
Copy link
Collaborator

famz commented Feb 16, 2024

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

@matttbe
Copy link
Author

matttbe commented Feb 16, 2024

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.

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.

A new test has been added to validate the new option, when disabled. No
need to add a new test for the default case (True) as it is already
implicitly tested before.

Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
@matttbe
Copy link
Author

matttbe commented Feb 16, 2024

@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
Copy link
Author

matttbe commented Feb 16, 2024

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants