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

Authorize /lgtm and /approve with OWNERS file #51

Merged
merged 5 commits into from
May 13, 2022

Conversation

carolynvs
Copy link
Contributor

When an OWNERS file is present, the /lgtm and /approve commands are authorized using the contents of the file instead of org/repo membership. The github action does not checkout the repository itself to locate OWNERS. The workflow is responsible for checking out the repo first before calling the prow action.

The format of the OWNERS file is a subset of prow's OWNERS file. I am only looking for the file at the root of the repository. The format is as follows:

approvers:
- user1
- user2

reviewers:
- user1
- user2
- user3

/lgtm checks if the commenter is in the reviewers section, and /approve checks if the commenter is in the approvers section. Membership in approvers does not authorize the use of /lgtm.

Closes #44

@carolynvs carolynvs force-pushed the owners-file branch 3 times, most recently from 07e48d8 to 2e29a8b Compare April 16, 2022 02:29
When an OWNERS file is present, the /lgtm and /approve commands are
authorized using the contents of the file instead of org/repo
membership. The github action does not checkout the repository itself to
locate OWNERS. The workflow is responsible for checking out the repo
first before calling the prow action.

The format of the OWNERS file is a subset of prow's OWNERS file.
I am only looking for the file at the root of the repository.
The format is as follows:

```yaml
approvers:
- user1
- user2

reviewers:
- user1
- user2
- user3
```

/lgtm checks if the commentor is in the reviewers section, and /approve
checks if the commentor is in the approvers section. Membership in
approvers does not authorize the use of /lgtm.

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
@carolynvs carolynvs marked this pull request as ready for review April 16, 2022 02:42
@carolynvs carolynvs requested a review from jpmcb as a code owner April 16, 2022 02:42
@carolynvs
Copy link
Contributor Author

If you'd like to see where this was tested, I've been chatting with the prow action on this test PR: ladygogo/test#1

@jpmcb
Copy link
Owner

jpmcb commented Apr 19, 2022

Oooo excellent! Thanks for this

Planning to get eyes on this week. Again, thank you so so much for your contribution!

@carolynvs
Copy link
Contributor Author

I have been a bit stumped by the one test failure. I'm unsure what I changed that is causing it.

FAIL __tests__/main.test.ts
  ● runs with no options

    Command failed: node /home/runner/work/prow-github-actions/prow-github-actions/lib/main.js

      10 |   }
      11 | 
    > 12 |   expect(cp.execSync(`node ${ip}`, options).toString()).toContain(
         |             ^
      13 |     'not yet supported'
      14 |   )
      15 | })

Copy link
Owner

@jpmcb jpmcb left a comment

Choose a reason for hiding this comment

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

This is looking amazing!! 👏🏼

Thank you so so much for this huge feature add.

I'm seeing a few prettier errors being thrown on running npm run all. I think once those are addressed, we can get this in

@jpmcb
Copy link
Owner

jpmcb commented Apr 22, 2022

I have been a bit stumped by the one test failure. I'm unsure what I changed that is causing it.

The github actions are actually broken for some reason. That's been a long standing problem that I haven't had a ton of time to look into. For example, it's broken in dependabot bumps. I'll make a new issue.

Running the tests locally look good for me:

npm run test-all

...

Test Suites: 25 passed, 25 total
Tests:       6 skipped, 81 passed, 87 total
Snapshots:   0 total
Time:        7.411 s
Ran all test suites.

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
@carolynvs
Copy link
Contributor Author

Alright, prettier has been appeased! Thanks for pointing that out. I'm new to js/ts and am still figuring out what all I should be running locally.

src/utils/auth.ts Outdated Show resolved Hide resolved
src/utils/auth.ts Outdated Show resolved Hide resolved
src/utils/auth.ts Outdated Show resolved Hide resolved
src/utils/auth.ts Outdated Show resolved Hide resolved
src/labels/lgtm.ts Show resolved Hide resolved
Owners all by itself could mean the repository owners. I've updated the
function names that work with the OWNERS file to clarify that they are
specifically for the file itself.

Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
Signed-off-by: Carolyn Van Slyck <me@carolynvanslyck.com>
@carolynvs
Copy link
Contributor Author

I've updated the PR to use "OwnersFile" instead of just "Owners" all by itself, and to retrieve the owners file via the API instead of cloning the repo.

@carolynvs
Copy link
Contributor Author

@jpmcb When you have time to review, let me know if there are more changes I need to make. I have a patch ready that replies to the PR when you don't have permission to run /lgtm or /approve so that people know when things aren't working.

Copy link
Owner

@jpmcb jpmcb left a comment

Choose a reason for hiding this comment

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

This is great. Let's bring this in!

@jpmcb jpmcb merged commit 95dde0c into jpmcb:main May 13, 2022
@carolynvs carolynvs deleted the owners-file branch May 13, 2022 16:29
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.

Support OWNERS files
2 participants