-
Notifications
You must be signed in to change notification settings - Fork 733
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
feat: add ability to add checks with app ids #1844
Conversation
40b3783
to
78778a2
Compare
78778a2
to
c8c7b30
Compare
e4a7af4
to
33e1ccb
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1844 +/- ##
============================================
+ Coverage 80.64% 80.70% +0.06%
- Complexity 2358 2363 +5
============================================
Files 225 225
Lines 7203 7221 +18
Branches 395 395
============================================
+ Hits 5809 5828 +19
+ Misses 1149 1148 -1
Partials 245 245 ☔ View full report in Codecov by Sentry. |
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.
Minor tweaks. Thanks!
src/main/java/org/kohsuke/github/GHBranchProtectionBuilder.java
Outdated
Show resolved
Hide resolved
src/main/java/org/kohsuke/github/GHBranchProtectionBuilder.java
Outdated
Show resolved
Hide resolved
src/main/java/org/kohsuke/github/GHBranchProtectionBuilder.java
Outdated
Show resolved
Hide resolved
private StatusChecks getStatusChecks() { | ||
if (statusChecks == null) { | ||
statusChecks = new StatusChecks(); | ||
statusChecks = new StatusChecksWithAppId(); | ||
} | ||
return statusChecks; | ||
} |
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.
Suggest something like this:
<T extends StatusCheck> T getStatusChecks(Class<T> clazz) {
if (statusChecks == null) {
try {
statusChecks = clazz.getDeclaredConstructor().newInstance();
} catch (Exception ignored) {
}
}
if (!clazz.isInstance(statusChecks) {
throw new GHException("Cannot use checks and context status checks");
}
return (T) statusChecks;
}
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.
Apparently the deprecated API can be implemented in the new API. From the docs:
I underlined the same logic between the old and new API.
I.e. you need to omit the app_id
field to do it the old way. I've added the @JsonInclude(JsonInclude.Include.NON_NULL)
to instruct jackson to omit it in that case, and a comment explaining the special "app_id" values.
So I committed a change which allows the use of both the old and new user methods at the same time, preserving both their effects.
How does that look to you?
Description
Fixes #1467
Add ability to receive and specify App IDs for required checks in branch protection rules.
Before submitting a PR:
@link
JavaDoc entries to the relevant documentation on https://docs.github.com/en/rest .mvn -D enable-ci clean install site
locally. If this command doesn't succeed, your change will not pass CI.main
. You will create your PR from that branch.When creating a PR: