Skip to content

Conversation

@iarspider
Copy link
Contributor

@iarspider iarspider commented Apr 8, 2025

@smuzaffar as discussed

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 8, 2025

A new Pull Request was created by @iarspider for branch master.

@cmsbuild, @iarspider, @smuzaffar can you please review it and eventually sign? Thanks.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.
cms-bot commands are listed here

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 8, 2025

cms-bot internal usage

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 8, 2025

Pull request #2471 was updated.

process_pr.py Outdated
logger.debug("All Parameters: %s", global_test_params)
# For now, only trigger tests for cms-sw/cmssw and cms-sw/cmsdist
if create_test_property:
if create_test_property and bot_status is not None and not is_closed_pr:
Copy link
Contributor

Choose a reason for hiding this comment

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

@iarspider , checking for bot_status has no use here. It might be empty for first run but on next bot run it will have some statuses . all of your current changes are not needed. A simple change like

if pr.state == "closed":
  create_test_property=False

after https://github.com/cms-sw/cms-bot/blob/master/process_pr.py#L1080 line will stop running PR tests for closed PRs.

I also wanted to avoid creating new statuses if PR is closed and old statuses are empty. It could be that there is bug on github side and they restore the old commit statuses at some point. If we create new statuses then they might not be able to restore them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see. I will now work on the second part of this PR (not updating statuses).

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 8, 2025

Pull request #2471 was updated.

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 9, 2025

Pull request #2471 was updated.

@iarspider iarspider requested a review from smuzaffar April 9, 2025 12:01
@smuzaffar
Copy link
Contributor

smuzaffar commented Apr 11, 2025

instead of uses create_status flag everywhere, as discussed, please try to override the commit status creation function

@cmsbuild
Copy link
Contributor

Pull request #2471 was updated.

@smuzaffar
Copy link
Contributor

@iarspider , I have tested this for cms-sw/cmssw#40600 and it worked fine but it did change the label to test-started though it properly did not create the property file for test to start. Can we avoid changing test label too for such PRs?

@iarspider
Copy link
Contributor Author

iarspider commented Apr 11, 2025

@smuzaffar so if we don't create status, we also don't change test labels, right? We do want to change other labels (e.g. x-approved)?

@smuzaffar
Copy link
Contributor

@iarspider , not exactly. We will still allow to change labels ( e.g. when someone signs a already merged PR) but we should avoid changing test labels for closed PRs

@iarspider
Copy link
Contributor Author

@iarspider , not exactly. We will still allow to change labels ( e.g. when someone signs a already merged PR) but we should avoid changing test labels for closed PRs

Understood.

@smuzaffar
Copy link
Contributor

we should avoid changing test labels for closed PRs

@iarspider , Note that we do want bot to update the tests labels for PRs for which release managers has merged while tests are still running. So we should avoid changing test labels for closed PRs which also have no commit statuses .

@cmsbuild
Copy link
Contributor

Pull request #2471 was updated.

@cmsbuild
Copy link
Contributor

Pull request #2471 was updated.

@smuzaffar
Copy link
Contributor

+externals

looks good, tested at cms-sw/cmssw#40598 and cms-sw/cmssw#40599 and bot did not set commit statuses or changed test labels

@smuzaffar smuzaffar merged commit 7fe35cb into master Apr 11, 2025
8 of 9 checks passed
@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. This pull request will now be reviewed by the release team before it's merged. @mandrenguyen, @antoniovilela, @rappoccio, @sextonkennedy (and backports should be raised in the release meeting by the corresponding L2)

@smuzaffar smuzaffar deleted the no-test-closed branch April 12, 2025 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants