-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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 Webhook for Repository ruleset #3305
Add Webhook for Repository ruleset #3305
Conversation
@gmlewis please ignore the previous PR and review this. This PR contains just my code and hasn't been reabased with master branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to hold off reviewing this one until comments from the last PR are addressed.
Specifically,
Godocs should always use complete sentences with punctuation so that the auto-generated package documentation formats and reads nicely.
It looks like GitHub ate my other lengthy comment, unfortunately... but the gist of it was that every exported struct needs a Godoc-style comment to explain its purpose and usage for the auto-generated package documentation.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3305 +/- ##
==========================================
- Coverage 97.72% 93.39% -4.33%
==========================================
Files 153 172 +19
Lines 13390 11909 -1481
==========================================
- Hits 13085 11123 -1962
- Misses 215 692 +477
- Partials 90 94 +4 ☔ View full report in Codecov by Sentry. |
Got it, will add comments for every new struct added and will let you know |
@gmlewis please go ahead and review the code, I've added comments for all the new structs generated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @unamdev0 !
Just a few linter errors to clean up, please, and a few tweaks, then we should be ready for a second LGTM+Approval from any other contributor to this repo before merging.
Done @gmlewis, all the suggestion made have been added and test files have been modified accordingly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like there are a few more linter errors - you can see them in the GitHub user interface, I believe.
You should also be able to see them locally if you follow step 4 of CONTRIBUTING.md. |
Hi @gmlewis, I'm not able to see any linting issues shown in github interface, when running script/lint.sh. Other than that,I've made the change that was missed last time |
Ah, to see them locally you probably need to merge the latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've gone ahead and made suggestions that should clear up the linter errors.
@gmlewis added the suggestions, should I add the changes in generated files manually as well as shown in the github UI? |
No, you should never edit the auto-generated files. If you find problems in them (which I don't think there are any), then the code that generates them |
Can you please push those changes to the suggested changes? I'm not seeing them yet. |
Oh okay, I've pushed changes with period at the end of comments, and not modified any other file, can you please review once |
Co-authored-by: Oleksandr Redko <[email protected]>
Co-authored-by: Oleksandr Redko <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll go ahead and apply these as well.
Co-authored-by: Oleksandr Redko <[email protected]>
Bummer. I didn't get the spacing right. working on it... |
@unamdev0 - sorry for all the noise - I was trying to make it so that we wouldn't require more changes from you, but alas I was unsuccessful. Could you please do this for me again?
|
done @gmlewis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unit tests still failing. I'll see if I can fix them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merging.
Added Repository ruleset event
Fixes #3295