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 Hacktoberfest feature #134

Merged
merged 3 commits into from
Oct 9, 2019
Merged

Add Hacktoberfest feature #134

merged 3 commits into from
Oct 9, 2019

Conversation

burtonr
Copy link
Contributor

@burtonr burtonr commented Oct 2, 2019

Description

This change adds the new feature hacktoberfest
The goal of this feature is to find flyby pull requests
that are considered spam.
A pull request that is a first time contributor, not signed,
and only changing markdown (md) files is assumed to be spam.
If the contributor has read the contribution guide, they
would have signed their commit, and the handler would have
no affect.
Otherwise, the pull request is closed, and an invalid
label is added indicating that this PR should not be counted

Signed-off-by: Burton Rheutan rheutan7@gmail.com

Motivation and Context

  • I have raised an issue to propose this change (required)
    Discussed with Alex via Slack

How Has This Been Tested?

Added unit tests to verify the "markdown only" function works as expected.

Unfortunately, I was unable to deploy and test as my deployment of Derek was not able to access the secret for some reason. I've run out of time this evening to debug further.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.

We can update the documentation once this feature passes an Alpha/Beta phase of testing

This change adds the new feature hacktoberfest
The goal of this feature is to find flyby pull requests
that are considered spam.
A pull request that is a first time contributor, not signed,
and only changing markdown (md) files is assumed to be spam.
If the contributor has read the contribution guide, they
would have signed their commit, and the handler would have
no affect.
Otherwise, the pull request is closed, and an invalid
label is added indicating that this PR should not be counted

Signed-off-by: Burton Rheutan <rheutan7@gmail.com>
@burtonr
Copy link
Contributor Author

burtonr commented Oct 2, 2019

I've built the container with this change available here: burtonr/derek:pr-hacktoberfest

@burtonr
Copy link
Contributor Author

burtonr commented Oct 3, 2019

Just found out that the fetchPullRequestCommits() does not return the list of files, although it is available in the struct.
I will need to add another method for getting the files.

This new function will get a list of the files included
in the pull request. This can be used to validate or
verify what files are being changed in a particular PR.

This commit adds that functionality to the hacktoberfest
feature to check for typo only pull requests only changing
markdown (md) files

Signed-off-by: Burton Rheutan <rheutan7@gmail.com>
@burtonr
Copy link
Contributor Author

burtonr commented Oct 4, 2019

@alexellis I've updated the changes to include the new fetchPullRequestFiles() function. The updated image is here: burtonr/derek:pr-hacktoberfest3

Tested with a lot of support from @rgee0 💯

Screenshot from 2019-10-03 21-05-13

To confirm this will not affect legitimate, signed, PRs:

Screenshot from 2019-10-03 21-10-26

@alexellis
Copy link
Owner

The screenshots look good. I would like to see a message from Derek explaining as briefly as possible what happened / why. This is to mitigate honest mistakes

Signed-off-by: Burton Rheutan <rheutan7@gmail.com>
@burtonr
Copy link
Contributor Author

burtonr commented Oct 4, 2019

I've added a message to the handler with a link to the contributing guide as well as the Hacktoberfest "Quality Standards" here: https://hacktoberfest.digitalocean.com/details#quality-standards

Available in the image burtonr/derek:pr-hacktoberfest5

Screenshot from 2019-10-04 08-28-00

@burtonr
Copy link
Contributor Author

burtonr commented Oct 4, 2019

As a "good to know", the Hacktoberfest site does recognize the "spam" tagged by this feature

Screenshot from 2019-10-04 08-42-26

listOpts := &github.ListOptions{
Page: 0,
}
commitFiles, resp, err := client.PullRequests.ListFiles(ctx, req.Repository.Owner.Login, req.Repository.Name, req.PullRequest.Number, listOpts)
Copy link
Owner

Choose a reason for hiding this comment

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

Are we sure that isn't part of the JSON already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we found that out the hard way. The check wasn't running. After looking at the request from GH, we saw that the files were a separate call

Copy link
Owner

Choose a reason for hiding this comment

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

I was wondering if we could avoid the extra API call to check files.

Copy link
Owner

Choose a reason for hiding this comment

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

Do any files get downloaded, or is it just a list?

fetchPullRequestFiles makes me feel like the disk is written too. If it's not written to, can we just go with listPullRequestFiles or whatever you prefer?

}

fmt.Println("Rate limiting", resp.Rate)
resp.Body.Close()
Copy link
Owner

Choose a reason for hiding this comment

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

Please use

if resp.Body!=nil {
  defer resp.Body.Close()
}

@alexellis
Copy link
Owner

This is looking good.

Can we show there is no regression? Does it still work with the regular pull request use-case for DCO and for labels?

Just the user guide needs updating now, in an additional commit please.

Copy link
Owner

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

Approved. Looks great, thanks for collaborating both @rgee0 @burtonr

@alexellis
Copy link
Owner

I'll merge so we can start trying it out. Please take a look at the naming question and send another PR to amend, also update the user guide.

@alexellis alexellis merged commit 9171922 into alexellis:master Oct 9, 2019
@burtonr
Copy link
Contributor Author

burtonr commented Oct 9, 2019

Thank you!
I'll do those minor updates and the user guide this evening in a new PR

@alexellis
Copy link
Owner

@burtonr burtonr deleted the hacktoberfest-feature branch October 10, 2019 03:26
@burtonr burtonr mentioned this pull request Oct 10, 2019
11 tasks
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.

2 participants