GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Close stale PRs without signed CLA by erlend-aasland · Pull Request #30500 · python/cpython

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented Jan 9, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

See python/core-workflow#93

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Erlend E. Aasland
Close stale PRs without signed CLA
05572d2
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jan 9, 2022
@bedevere-bot bedevere-bot added the awaiting review label Jan 9, 2022
@erlend-aasland
erlend-aasland requested review from Mariatta, ambv and brettcannon January 9, 2022 12:30
Erlend E. Aasland
Address Łukasz' remarks
b14cf55
@erlend-aasland erlend-aasland added skip issue skip news labels Jan 9, 2022
@hugovk hugovk mentioned this pull request Jan 9, 2022
Cron job to close stale PRs python/core-workflow#93
Closed
@erlend-aasland
erlend-aasland requested a review from hugovk January 9, 2022 15:39
@erlend-aasland

erlend-aasland commented Jan 9, 2022

Copy link
Copy Markdown
Contributor Author

FYI, we're "speed-testing" this PR over at https://github.com/hugovk/cpython/pulls

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Jan 9, 2022
Test stale-action, PR 2 - with "fake CLA signed" label hugovk/cpython#18
Closed
Test stale-action, PR 1 - with "fake CLA not signed" label hugovk/cpython#17
Closed
sumeshir26
sumeshir26 approved these changes Jan 10, 2022
View reviewed changes
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Jan 10, 2022
@erlend-aasland

erlend-aasland commented Jan 10, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@hugovk, based on the first cron run, I suggest the following changes:

  1. Based on the debug output, use only-pr-labels: 'CLA signed' instead of exempt-pr-labels: 'CLA not signed' for the first step. It is probably easier to read as well.
  2. Name each step. Currently both of them confusingly appear as Run actions/stale@v4.

Actually, I found the step with the "exempt rule" confusing to read compared to the step with the "only rule". It seems like the two rules are processed differently as well. For only-pr-labels, the action aborts almost immediately. For exempt-pr-labels it appears to abort much later in the process.

hugovk reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Erlend E. Aasland added 2 commits January 10, 2022 10:16
Erlend E. Aasland
Name each step for improved logging
f405b93
Erlend E. Aasland
Use only-pr-labels iso. exempt-pr-labels
26028a5
@erlend-aasland

erlend-aasland commented Jan 10, 2022

Copy link
Copy Markdown
Contributor Author

Actually, I found the step with the "exempt rule" confusing to read compared to the step with the "only rule". It seems like the two rules are processed differently as well. For only-pr-labels, the action aborts almost immediately. For exempt-pr-labels it appears to abort much later in the process.

Debug output from the PR which was caught by the exempt-rule
[#17] Pull request #17
[#17] Found this pull request last updated at: 2022-01-09T16:25:57Z
[#17] The option only-labels (​https://github.com/actions/stale#only-labels​) was not specified
[#17] └── Continuing the process for this pull request
[#17] Days before pull request stale: 1
[#17] The pull request is not closed nor locked. Trying to remove the close label...
[#17] ├── The close-pr-label (​https://github.com/actions/stale#close-pr-label​) option was not set
[#17] └── Skipping the removal of the close label
[#17] This pull request hasn't a stale label
[#17] Skipping this pull request because it has an exempt label
Debug output from the PR which was caught by the only-rule
[#18] Pull request #18
[#18] Found this pull request last updated at: 2022-01-09T16:25:48Z
[#18] The option only-labels (​https://github.com/actions/stale#only-labels​) was specified to only process issues and pull requests with all those labels (1)
[#18] └── Skipping this pull request because it doesn't have all the required labels

I prefer the latter.

hugovk reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hugovk
hugovk reviewed Jan 10, 2022
View reviewed changes
Comment thread .github/workflows/stale.yml Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread .github/workflows/stale.yml Resolved

Uh oh!

There was an error while loading. Please reload this page.

Erlend Egeberg Aasland and others added 2 commits January 10, 2022 11:24
Erlend Egeberg Aasland @hugovk
Plural, not genitive
007570b
Co-authored-by: Hugo van Kemenade <[email protected]>
Erlend Egeberg Aasland @hugovk
Plural, not genitive
258a475
Co-authored-by: Hugo van Kemenade <[email protected]>
Mariatta
Mariatta approved these changes Jan 10, 2022
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Jan 10, 2022
@erlend-aasland erlend-aasland added the DO-NOT-MERGE label Jan 11, 2022
@erlend-aasland

erlend-aasland commented Jan 11, 2022

Copy link
Copy Markdown
Contributor Author

@Mariatta, thank you for reviewing! I've added the do-not-merge label until we've tested this change properly on our forks (see the last TODO item). I'll remove that label if everything is all fine after we've finished testing.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Jan 11, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@hugovk, the logs look much better with the recent changes applied: https://github.com/hugovk/cpython/runs/4769591131?check_suite_focus=true

hugovk reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland
erlend-aasland removed the request for review from brettcannon January 12, 2022 22:40
@erlend-aasland

erlend-aasland commented Jan 13, 2022

Copy link
Copy Markdown
Contributor Author

I've tested this change on my fork, modified to mark a PR stale after 1 day, and close after 1 day (no CLA). I've verified, as expected, that PRs that are marked stale, and that received comments after being marked stale, will not be closed (cc. @ambv).

For test PR 20, which received comments after being marked stale, the actions bot correctly removed the stale label on the following run. For test PR 21, which did not receive comments after being marked stale, the actions bot closed the PR on the following run.

Here's the PRs:

  • Test: CLA signed erlend-aasland/cpython#19
  • Test CLA not signed, don't touch erlend-aasland/cpython#20
  • Test CLA not signed, touch post stale erlend-aasland/cpython#21

(Removing the do-not-merge label, @Mariatta.)

ambv and hugovk reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland erlend-aasland removed the DO-NOT-MERGE label Jan 13, 2022
hugovk
hugovk approved these changes Jan 15, 2022
View reviewed changes

@hugovk hugovk left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Worked as expected on hugovk#17 and hugovk#18 👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
raghavthind2005
raghavthind2005 approved these changes Jan 18, 2022
View reviewed changes
@erlend-aasland

erlend-aasland commented Feb 5, 2022

Copy link
Copy Markdown
Contributor Author

@ambv Are ok with this PR as it stands?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ambv

ambv commented Feb 17, 2022

Copy link
Copy Markdown
Contributor

@erlend-aasland yes, let's land it.

erlend-aasland and hugovk reacted with hooray emoji
All reactions
  • 🎉 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ambv
ambv merged commit 5bc4327 into python:main Feb 17, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Feb 17, 2022
@erlend-aasland
erlend-aasland deleted the close-stale-non-cla branch February 17, 2022 08:23
@arhadthedev arhadthedev mentioned this pull request Apr 10, 2022
Remove dead "Check PRs with 'CLA not signed' label" #91429
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hugovk hugovk hugovk approved these changes
@Mariatta Mariatta Mariatta approved these changes
@ambv ambv Awaiting requested review from ambv
+2 more reviewers
@sumeshir26 sumeshir26 sumeshir26 approved these changes
@raghavthind2005 raghavthind2005 raghavthind2005 approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip issue skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@erlend-aasland @ambv @hugovk @Mariatta @sumeshir26 @raghavthind2005 @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.