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

Add CODEOWNERS and fix+extend mergeable config #4963

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

pkuczynski
Copy link
Member

  • add CODEOWNERS
  • fix meargeble config issues
  • add automatic assigning
  • add validating aprovals
  • add handling stale PR

@mergeable
Copy link

mergeable bot commented Jul 10, 2020

This PR does not contain all required information and changes. Please review the mergeable status.
If you are member of the RVM team, ensure that you have applied all necessary labels, milestones, etc.

@mergeable
Copy link

mergeable bot commented Jul 10, 2020

This PR does not contain all required information and changes. Please review the mergeable status.

If you are member of the RVM team, ensure that you have applied all necessary labels, milestones, etc.

@mergeable
Copy link

mergeable bot commented Jul 10, 2020

This PR does not contain all required information and changes. Please review the mergeable status.

If you are member of the RVM team, ensure that you have applied all necessary labels, milestones, etc.

@pkuczynski pkuczynski added this to the rvm-1.29.11 milestone Jul 10, 2020
@mergeable
Copy link

mergeable bot commented Jul 10, 2020

This PR does not contain all required information and changes. Please review the mergeable status.

If you are member of the RVM team, ensure that you have applied all necessary labels, milestones, etc.

@@ -0,0 +1 @@
* @rvm/maintainers @rvm/contributors

Choose a reason for hiding this comment

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

i'll double check if mergeable has implemented team member functionality for CODEOWNERS. I suspect that this will not work as expected with mergeable currently.

Copy link
Member Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

it's great if it works xD

Copy link
Member Author

Choose a reason for hiding this comment

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

At least that's what the documentation says...

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually seems it does not work: https://github.com/rvm/rvm/pull/4963/checks?check_run_id=858856086

✔️ approvals: all required reviewers have approved
Input :
Settings : {"required":{"owners":true,"assignees":true}}

No owner approved yet! However maybe its reading CODEOWNERS from master and not from PR?

Choose a reason for hiding this comment

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

hmm.. it's as i expected, passing in a team name instead of individual name requires extra code which is not implemented in mergeable yet.

Choose a reason for hiding this comment

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

At least that's what the documentation says...

yeah, CODEOWNERS should work for individuals for sure. we didn't account for teams.

Choose a reason for hiding this comment

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

test comment

Copy link
Member Author

Choose a reason for hiding this comment

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

@shine2lay have this been fixed on your side maybe?

Choose a reason for hiding this comment

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

yes, this feature has been implemented

pass:
- do: comment
payload:
body: There was no activity for 20 dats. Is this still relevant?

Choose a reason for hiding this comment

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

typo s/dats/days

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks :)

@pkuczynski
Copy link
Member Author

@shine2lay I didn't realized it would create separate checks when I add "names"

@shine2lay
Copy link

shine2lay commented Jul 10, 2020

@pkuczynski each recipe (- when) is considered independent will execute it's own actions (i.e checks and comments) If you wish to have a single check, you'll have to lump the validators under one recipe

@pkuczynski pkuczynski modified the milestones: rvm-1.29.11, rvm-1.29.12 Dec 29, 2020
@pkuczynski pkuczynski modified the milestones: rvm-1.29.12, rvm-1.29.13 Jan 15, 2021
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.

None yet

2 participants