Skip to content
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

author-can-merge: false does not seem to work #123

Closed
jrfnl opened this issue Dec 4, 2023 · 13 comments · Fixed by #124 or #126
Closed

author-can-merge: false does not seem to work #123

jrfnl opened this issue Dec 4, 2023 · 13 comments · Fixed by #124 or #126
Labels
bug Something isn't working

Comments

@jrfnl
Copy link

jrfnl commented Dec 4, 2023

Hiya,

As one of the criteria in a labeler.yml file, I have the following:

 labels:
   - label: "Status: triage"
     draft: false
     author-can-merge: false

I'd expect

The Status: triage label to be added to all non-draft PRs where the author doesn't have commit rights in the repo.

What actually happened

The Status: triage label does not get added to PRs from outside contributors (who don't have commit rights to the repo).

Other info

I haven't tested yet if it works when I set it as follows, but if it does, I'd still find that counter-intuitive, as I'd expect author-can-merge: false to be respected.

labels:
  - label: "Status: triage"
    negate: true
    draft: false
    author-can-merge: true
@srvaroa
Copy link
Owner

srvaroa commented Dec 4, 2023

Hi @jrfnl, what version are you using?

@srvaroa srvaroa added the bug Something isn't working label Dec 4, 2023
@srvaroa
Copy link
Owner

srvaroa commented Dec 4, 2023

Nevermind, I just pushed a fix. I've added an extra test with the final condition you wanted to set.

@srvaroa
Copy link
Owner

srvaroa commented Dec 4, 2023

Should be fixed in https://github.com/srvaroa/labeler/releases/tag/v1.8.1

Thanks for reporting. Let me know if this works as expected

@jrfnl
Copy link
Author

jrfnl commented Dec 4, 2023

Thank you for the quick response @srvaroa ! I am using srvaroa/labeler@v1, so the fix should come in automatically and as the repo I'm using it in is hyper-active at this moment (I'm taking over a big project in a fork, so people are porting their old PRs to the new repo), I should be able to let you know how things are going soon!

@srvaroa
Copy link
Owner

srvaroa commented Dec 4, 2023

I will update v1 to point at v1.8.1 first thing tomorrow!

@srvaroa
Copy link
Owner

srvaroa commented Dec 5, 2023

v1 is updated

tylerbutler added a commit to microsoft/FluidFramework that referenced this issue Dec 6, 2023
All PRs are being labeled "external-contribution". This is probably
because of srvaroa/labeler#123, which has been
resolved in the latest v1 release. I updated the version of the action
to the latest v1 version.
@jrfnl
Copy link
Author

jrfnl commented Dec 9, 2023

@srvaroa I've been checking new PRs for a few days now and something still doesn't seem right.

The "triage" label now gets added to all PRs, even those of people who can merge in the repo.

@srvaroa
Copy link
Owner

srvaroa commented Dec 9, 2023

Hi @jrfnl i will take a look asap. Is this in a public repo where I can take a look at the logs?

@jrfnl
Copy link
Author

jrfnl commented Dec 9, 2023

@srvaroa Appreciated and yes, the repo is public. Sorry, I didn't add links before as, while the repo is public, the news that the repo existed was not, but an announcement was made last week ;-)

Workflow file: https://github.com/PHPCSStandards/PHP_CodeSniffer/blob/master/.github/workflows/label-new-prs.yml
Config: https://github.com/PHPCSStandards/PHP_CodeSniffer/blob/master/.github/labeler.yml
Logs: https://github.com/PHPCSStandards/PHP_CodeSniffer/actions/workflows/label-new-prs.yml

Here is one where the "triage" label now correctly got added for a contributor who doesn't have commit: https://github.com/PHPCSStandards/PHP_CodeSniffer/actions/runs/7144614939/job/19458612350
And here is one where the "triage" label incorrectly got added to one of my own PRs, while I do have commit: https://github.com/PHPCSStandards/PHP_CodeSniffer/actions/runs/7148232941/job/19468964257

@srvaroa srvaroa reopened this Dec 10, 2023
srvaroa added a commit that referenced this issue Dec 10, 2023
author-can-merge was not considering all the roles that allow for
committing into the base branch

Fixes: #123

Signed-off-by: Galo Navarro <anglorvaroa@gmail.com>
srvaroa added a commit that referenced this issue Dec 10, 2023
author-can-merge was not considering all the roles that allow for
committing into the base branch

Fixes: #123

Signed-off-by: Galo Navarro <anglorvaroa@gmail.com>
@srvaroa
Copy link
Owner

srvaroa commented Dec 10, 2023

@jrfnl it looks like the implementation of that condition was not considering all the roles that allow for committing to the base branch, only OWNER.

This is released as v1.8.2, the v1 floating reference is also updated. Let me know if this makes things work as expected.

@jrfnl
Copy link
Author

jrfnl commented Dec 10, 2023

@jrfnl it looks like the implementation of that condition was not considering all the roles that allow for committing to the base branch, only OWNER.

Interesting as I would have expected that repo to identify me as an owner too.
I'm the owner of the organisation + the admin of the repo, which effectively makes me the owner.

Could this be because this repo was originally a fork and was "decoupled" ?

This is released as v1.8.2, the v1 floating reference is also updated. Let me know if this makes things work as expected.

Will do! Thanks @srvaroa for working on this!

@srvaroa
Copy link
Owner

srvaroa commented Dec 10, 2023

I noticed in the logs you linked above that the action resolved the author as non-owner, so I suspected this was the problem. The fix is needed anyway, but do let me know if it doesn't evaluate as expected for your PRs and I'll look further into it.

Thanks for the bug reports!

@jrfnl
Copy link
Author

jrfnl commented Dec 10, 2023

I noticed in the logs you linked above that the action resolved the author as non-owner, so I suspected this was the problem. The fix is needed anyway, but do let me know if it doesn't evaluate as expected for your PRs and I'll look further into it.

Thanks for the bug reports!

It may be a few days/week before I have enough info to evaluate fully.

There are basically three kind of PRs I'd like to see come in to be able to say for sure:

  • One from me (should not get "triage" label).
  • One from a member in the organisation who has triage permission on the repo, but does not have commit (should get "triage" label).
  • One from an outside contributor to the repo (should get "triage" label).

Will let you know how I get on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants