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

Use team keys for authors #88

Merged
merged 10 commits into from
May 23, 2023

Conversation

hbrysiewicz
Copy link
Contributor

@hbrysiewicz hbrysiewicz commented May 1, 2023

It was not initially clear in README that teams:{gh-team-name} syntax was only used for reviewers. I tried using it for the per_author keys and it (obviously) didn't work. So I wrote this in so that it would work. If the PR author is in the per_author team:{gh-team-name} collection, use the respective reviewer for assignment.

reviewers:
  per_author:
    team:koopa-troop:
      - mario

For example, this configuration would assign reviewer mario when the PR author is in github team koopa-troop.

Copy link
Owner

@necojackarc necojackarc left a comment

Choose a reason for hiding this comment

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

Thanks for raising a PR! Looks good to me 👍 Clean code!
I'll update the dist and see if CI passes 🤞

src/reviewer.js Show resolved Hide resolved
src/reviewer.js Outdated Show resolved Hide resolved
test/stubs/context.js Show resolved Hide resolved
src/github.js Outdated Show resolved Hide resolved
src/github.js Outdated Show resolved Hide resolved
src/reviewer.js Outdated Show resolved Hide resolved
test/reviewer.test.js Outdated Show resolved Hide resolved
@necojackarc
Copy link
Owner

necojackarc commented May 4, 2023

I can't push commits to this branch, so I've made this PR #89 but it's still half-done.

@hbrysiewicz
Copy link
Contributor Author

@necojackarc I pushed updates per PR comments.

src/github.js Outdated
Comment on lines 120 to 123
return octokit.teams.listMembersInOrg({
org: context.repo.org,
team_slug: team,
})?.map((member) => member.login)
Copy link
Owner

Choose a reason for hiding this comment

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

Have you been able to check if it returns this in this format?
I haven't got around to but I wonder it may return a response in a data property 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://docs.github.com/en/rest/teams/members?apiVersion=2022-11-28#list-team-members
It does look like it returns an array. From the documentation the schema doesn't appear to be in a data property

Copy link
Owner

Choose a reason for hiding this comment

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

This is the document for the REST API wrapped in octokit, so it doesn't tell us how octokit returns its response. It may or may not have data. octokit.repos.getContent returns a response with a data property, so I suspect octokit.teams.listMembersInOrg may do the same thing, but I'm not sure yet. Need a way to play around with this API... or the official document (or reading the code...).

const { data: response_body } = await octokit.repos.getContent({
owner: context.repo.owner,
repo: context.repo.repo,
path: config_path,
ref: context.ref,
});

@hbrysiewicz
Copy link
Contributor Author

Just bumping on this to see if I can help move it forward at all

@necojackarc
Copy link
Owner

@hbrysiewicz Sorry for the late reply. I didn't manage to get around to GitHub until today.

Your idea, design and implementation seem all good. I'm happy to help this PR get merged, but I can't push changes to your branch, so here's the first thing we have to do - could you run npm build to update dist? That's the current reason CI is failing.

Once you push it, we can see if all the tests are running fine.

@hbrysiewicz
Copy link
Contributor Author

Build has been pushed

@hbrysiewicz
Copy link
Contributor Author

@necojackarc waiting for maintainer to approve the test run

@necojackarc
Copy link
Owner

@hbrysiewicz Thanks. I need to find a way to let contributors run GitHub actions...
Perhaps, I need to use pull_request_target, not pull_request, off the top of my head.

@hbrysiewicz
Copy link
Contributor Author

@necojackarc Just pushed up the eslint fixes if you can run again.

@necojackarc necojackarc mentioned this pull request May 22, 2023
2 tasks
@necojackarc
Copy link
Owner

@hbrysiewicz Please could you back-merge the latest master to your branch so that you can run the CI workflows without approval? I believe this works by looking at the document (I've also added Node v18 to CI). #90

@hbrysiewicz
Copy link
Contributor Author

@necojackarc All done

src/github.js Outdated Show resolved Hide resolved
Copy link
Owner

@necojackarc necojackarc left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your contribution!

Copy link
Owner

@necojackarc necojackarc left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your contribution!

@necojackarc necojackarc merged commit bf71b47 into necojackarc:master May 23, 2023
3 checks passed
@hbrysiewicz
Copy link
Contributor Author

@necojackarc it looks like context.repo.org is not working on line 121 src/github.js. Do you know where docs for this context might be or where the correct location of org is?

@necojackarc
Copy link
Owner

@hbrysiewicz Let me revert this first since this action is used in more than 400 repos, so I can't keep the latest version broken. Let's discuss it in the next PR you created.

necojackarc added a commit that referenced this pull request May 24, 2023
necojackarc added a commit that referenced this pull request May 24, 2023
* Revert "Add one example to show the new feature"

This reverts commit 4bc5b46.

* Revert "Fix the stub to return in the expected format (#93)"

This reverts commit a39c70b.

* Revert "Use team keys for authors (#88)"

This reverts commit bf71b47.
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.

None yet

2 participants