Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Serverless based Lambda function for Labelling PRs based on CI & PR Review status #27

Merged
merged 29 commits into from Oct 12, 2020

Conversation

ChaiBapchya
Copy link
Contributor

@ChaiBapchya ChaiBapchya commented Jul 26, 2020

Purpose

An automated way of labeling PRs based on CI status & PR Review state.
This serves dual purpose

  • Automates the labeling
  • Allows contributors to prioritize review of PRs.

Testing

Tested it locally & on dev account.
Deployed this on Prod account.

Reproduce

Once you put the redacted code & have AWSCLI + Serverless configured

./deploy_lambda.sh

Should work for deploying this lambda function on the account [dev/prod stage based on choice].

** Note **
Secrets have been redacted.

Approach

Details

Github Webhook -> Lambda 1 -> SQS -> Lambda 2
Github Webhook sends Status Events & triggers the lambda function "pr-status-labeler"
Lambda 2 in turn leverages Github API to add labels to PR [based on the following logic]

if WIP in title or PR is draft or CI failed:
    pr-work-in-progress
elif CI has not started yet or CI is in progress:
    pr-awaiting-testing
else: # CI passed checks
    if pr has at least one approval and no request changes:
        pr-awaiting-merge
    elif pr has no review or all reviews have been dismissed/re-requested:
        pr-awaiting-review
    else: # pr has a review that hasn't been dismissed yet no approval
        pr-awaiting-response

Old Approach [Use Jenkins API + GithubAPI]

Details

Runs once per day. Configured in the serverless.yml script.
It reuses credentials set during MXNet Bot [Jenkins & Github credentials].

Results

Results in the successful run on Prod for apache/incubator-mxnet repo

~~Open PRs : 207~~
~~PRs labeled : 18~~
~~PRs already labeled : 38~~
~~PRs with unknown jobs : 2~~
~~PRs with status failed : 149~~

Lambda function labeled 18 PRs that had passed all the test. Detailed logs in the Cloudwatch Log Group [corresponding to the Lambda function] in the Prod CI AWS account]

Copy link
Contributor

@marcoabreu marcoabreu left a comment

Choose a reason for hiding this comment

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

I'm not really sure about the objective of this, but I see quite a few issues:

  • you don't need to query Jenkins at all, GitHub has all the information
  • once a PR went into a successful status, the label will not be redacted
  • you should rather use an event driven approach where you subscribe to the GitHub webhook Event which is fired every single time a commit is made or check has been updated. Then have a label like "ci-passed", "unstable" and "ci-unsuccesful".

Right now I think that this implementation will cause rather more confusion.

services/lambda-label-successful-prs/CIBot.py Outdated Show resolved Hide resolved
services/lambda-label-successful-prs/CIBot.py Outdated Show resolved Hide resolved
services/lambda-label-successful-prs/CIBot.py Outdated Show resolved Hide resolved
@szha
Copy link
Member

szha commented Jul 31, 2020

maybe we could automate the PR labeling as the following pseudo code describes:

if WIP in title or PR is draft or CI failed:
    pr-work-in-progress
elif CI has not started yet or CI is in progress:
    pr-awaiting-testing
else: # CI passed checks
    if pr has at least one approval and no request changes:
        pr-awaiting-merge
    elif pr has no review or all reviews have been dismissed/re-requested:
        pr-awaiting-review
    else: # pr has a review that hasn't been dismissed yet no approval
        pr-awaiting-response

Copy link
Contributor

@marcoabreu marcoabreu left a comment

Choose a reason for hiding this comment

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

Generally going in a great direction!

services/lambda-pr-status-labeler/PRStatusBot.py Outdated Show resolved Hide resolved
services/lambda-pr-status-labeler/PRStatusBot.py Outdated Show resolved Hide resolved
services/lambda-pr-status-labeler/PRStatusBot.py Outdated Show resolved Hide resolved
services/lambda-pr-status-labeler/PRStatusBot.py Outdated Show resolved Hide resolved
Copy link
Contributor

@marcoabreu marcoabreu left a comment

Choose a reason for hiding this comment

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

I think the approach to test should be reengineered. At the moment, this does not seem like a proper test procedure since things are not mocked out and neither are the results verified.

@sandeep-krishnamurthy
Copy link

Not valid review comments.
I just came here to thank you for the hard work on this feature and burning your weekend in keeping this feature developed.

@marcoabreu
Copy link
Contributor

What do you mean with "not valid review comments", Sandeep?

@sandeep-krishnamurthy
Copy link

I just meant - My comment is not a technical review comment on the PR. This is just a thank you note :)

@ChaiBapchya ChaiBapchya changed the title Serverless based Lambda function for Labelling Successful PRs with pr-awaiting-review Serverless based Lambda function for Labelling PRs based on CI & PR Review status Sep 8, 2020
@ChaiBapchya
Copy link
Contributor Author

To run all the tests

python -m pytest test_mock.py

Copy link
Member

@szha szha left a comment

Choose a reason for hiding this comment

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

remember to add apache header to all files.

Copy link
Contributor

@marcoabreu marcoabreu 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 work!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants