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.

Sign up for GitHub

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

Jump to bottom

feat: add ability to add checks with app ids #1844

Merged
merged 7 commits into from
Jun 15, 2024
Merged

feat: add ability to add checks with app ids #1844

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c8c7b30
feat: add ability to add checks with app ids
tginiotis-at-work May 15, 2024
33e1ccb
test: add test to verify new checks are being populated
tginiotis-at-work May 17, 2024
47314dd
fix: some post-review changes and test additions
tginiotis-at-work Jun 6, 2024
3882581
Merge branch 'main' into app_ids_for_checks
bitwiseman Jun 8, 2024
3d5196f
fix: allow null app ids to provide the same behaviour as the deprecat…
tginiotis-at-work Jun 13, 2024
6819f35
test: regenerate some mocks
tginiotis-at-work Jun 13, 2024
995bee2
Merge branch 'main' into app_ids_for_checks
bitwiseman Jun 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions src/main/java/org/kohsuke/github/GHBranchProtection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,51 @@ public boolean isEnabled() {
}
}

/**
* The type Check.
*/
public static class Check {
private String context;
private Integer appId;

/**
* no-arg constructor for the serializer
*/
public Check() {
}

/**
* Regular constructor for use in user business logic
*
* @param context
* the context string of the check
* @param appId
* the application ID the check is supposed to come from
*/
public Check(String context, Integer appId) {
this.context = context;
this.appId = appId;
}

/**
* The context string of the check
*
* @return the string
*/
public String getContext() {
return context;
}

/**
* The application ID the check is supposed to come from. The value "-1" indicates "any source".
*
* @return the integer
*/
public Integer getAppId() {
return appId;
}
}

/**
* The type AllowForcePushes.
*/
Expand Down Expand Up @@ -462,6 +507,9 @@ public static class RequiredStatusChecks {
@JsonProperty
private Collection<String> contexts;

@JsonProperty
private Collection<Check> checks;

@JsonProperty
private boolean strict;

Expand All @@ -477,6 +525,15 @@ public Collection<String> getContexts() {
return Collections.unmodifiableCollection(contexts);
}

/**
* Gets checks.
*
* @return the checks
*/
public Collection<Check> getChecks() {
return Collections.unmodifiableCollection(checks);
}

/**
* Gets url.
*
Expand Down
47 changes: 43 additions & 4 deletions src/main/java/org/kohsuke/github/GHBranchProtectionBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,27 @@
* the checks
* @return the gh branch protection builder
*/
public GHBranchProtectionBuilder addRequiredChecksWithAppIds(Collection<GHBranchProtection.Check> checks) {
tginiotis-at-work marked this conversation as resolved.
Show resolved Hide resolved
if (!(getStatusChecks() instanceof StatusChecksWithAppId)) {
statusChecks = new StatusChecksWithAppId();

Check warning on line 55 in src/main/java/org/kohsuke/github/GHBranchProtectionBuilder.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/kohsuke/github/GHBranchProtectionBuilder.java#L55 

Added line #L55 was not covered by tests
}
((StatusChecksWithAppId) getStatusChecks()).checks.addAll(checks);
tginiotis-at-work marked this conversation as resolved.
Show resolved Hide resolved
return this;
}

/**
* Add required checks gh branch protection builder.
*
* @param checks
* the checks
* @return the gh branch protection builder
*/
@Deprecated
public GHBranchProtectionBuilder addRequiredChecks(Collection<String> checks) {
getStatusChecks().contexts.addAll(checks);
if (!(getStatusChecks() instanceof StatusChecksDeprecated)) {
statusChecks = new StatusChecksDeprecated();
}
((StatusChecksDeprecated) getStatusChecks()).contexts.addAll(checks);
tginiotis-at-work marked this conversation as resolved.
Show resolved Hide resolved
return this;
}

Expand All @@ -62,11 +81,24 @@
* the checks
* @return the gh branch protection builder
*/
@Deprecated
public GHBranchProtectionBuilder addRequiredChecks(String... checks) {
addRequiredChecks(Arrays.asList(checks));
return this;
}

/**
* Add required checks gh branch protection builder.
*
* @param checks
* the checks
* @return the gh branch protection builder
*/
public GHBranchProtectionBuilder addRequiredChecksWithAppIds(GHBranchProtection.Check... checks) {
tginiotis-at-work marked this conversation as resolved.
Show resolved Hide resolved
addRequiredChecksWithAppIds(Arrays.asList(checks));
return this;
}

/**
* Allow deletion of the protected branch.
*
Expand Down Expand Up @@ -532,7 +564,7 @@

private StatusChecks getStatusChecks() {
if (statusChecks == null) {
statusChecks = new StatusChecks();
statusChecks = new StatusChecksWithAppId();
}
return statusChecks;
}
Copy link
Member

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;
    }

Copy link
Contributor Author

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:
image

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?

Expand All @@ -546,8 +578,15 @@
private Set<String> users = new HashSet<String>();
}

private static class StatusChecks {
final List<String> contexts = new ArrayList<String>();
private static abstract class StatusChecks {
boolean strict;
}

private static class StatusChecksWithAppId extends StatusChecks {
final List<GHBranchProtection.Check> checks = new ArrayList<>();
}

private static class StatusChecksDeprecated extends StatusChecks {
final List<String> contexts = new ArrayList<>();
}
}
25 changes: 25 additions & 0 deletions src/test/java/org/kohsuke/github/GHBranchProtectionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
import org.kohsuke.github.GHBranchProtection.RequiredReviews;
import org.kohsuke.github.GHBranchProtection.RequiredStatusChecks;

import java.util.ArrayList;

import static org.hamcrest.Matchers.*;

// TODO: Auto-generated Javadoc
Expand Down Expand Up @@ -189,6 +191,29 @@ public void testSignedCommits() throws Exception {
assertThat(protection.getRequiredSignatures(), is(false));
}

/**
* Checks with app ids are being populated
*
* @throws Exception
* the exception
*/
@Test
public void testChecksWithAppIds() throws Exception {
GHBranchProtection protection = branch.enableProtection()
.addRequiredChecksWithAppIds(new GHBranchProtection.Check("context", -1),
new GHBranchProtection.Check("context2", 123))
.enable();

ArrayList<GHBranchProtection.Check> resultChecks = new ArrayList<>(
protection.getRequiredStatusChecks().getChecks());

assertThat(resultChecks.size(), is(2));
assertThat(resultChecks.get(0).getContext(), is("context"));
assertThat(resultChecks.get(0).getAppId(), nullValue());
assertThat(resultChecks.get(1).getContext(), is("context2"));
assertThat(resultChecks.get(1).getAppId(), is(123));
}

/**
* Test get protection.
*
Expand Down
34 changes: 34 additions & 0 deletions ...g/kohsuke/github/GHBranchProtectionTest/wiremock/testChecksWithAppIds/__files/1-user.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
{
"login": "tginiotis-at-work",
"id": 61763026,
"node_id": "MDQ6VXNlcjYxNzYzMDI2",
"avatar_url": "https://avatars.githubusercontent.com/u/61763026?v=4",
"gravatar_id": "",
"url": "https://api.github.com/users/tginiotis-at-work",
"html_url": "https://github.com/tginiotis-at-work",
"followers_url": "https://api.github.com/users/tginiotis-at-work/followers",
"following_url": "https://api.github.com/users/tginiotis-at-work/following{/other_user}",
"gists_url": "https://api.github.com/users/tginiotis-at-work/gists{/gist_id}",
"starred_url": "https://api.github.com/users/tginiotis-at-work/starred{/owner}{/repo}",
"subscriptions_url": "https://api.github.com/users/tginiotis-at-work/subscriptions",
"organizations_url": "https://api.github.com/users/tginiotis-at-work/orgs",
"repos_url": "https://api.github.com/users/tginiotis-at-work/repos",
"events_url": "https://api.github.com/users/tginiotis-at-work/events{/privacy}",
"received_events_url": "https://api.github.com/users/tginiotis-at-work/received_events",
"type": "User",
"site_admin": false,
"name": "Tadas Giniotis",
"company": "IBM Lietuva",
"blog": "",
"location": null,
"email": null,
"hireable": null,
"bio": null,
"twitter_username": null,
"public_repos": 12,
"public_gists": 0,
"followers": 0,
"following": 0,
"created_at": "2020-03-03T23:04:00Z",
"updated_at": "2024-05-15T15:03:51Z"
}
156 changes: 156 additions & 0 deletions ...ProtectionTest/wiremock/testChecksWithAppIds/__files/2-r_h_temp-testcheckswithappids.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
{
"id": 802086844,
"node_id": "R_kgDOL87fvA",
"name": "temp-testChecksWithAppIds",
"full_name": "hub4j-test-org/temp-testChecksWithAppIds",
"private": false,
"owner": {
"login": "hub4j-test-org",
"id": 7544739,
"node_id": "MDEyOk9yZ2FuaXphdGlvbjc1NDQ3Mzk=",
"avatar_url": "https://avatars.githubusercontent.com/u/7544739?v=4",
"gravatar_id": "",
"url": "https://api.github.com/users/hub4j-test-org",
"html_url": "https://github.com/hub4j-test-org",
"followers_url": "https://api.github.com/users/hub4j-test-org/followers",
"following_url": "https://api.github.com/users/hub4j-test-org/following{/other_user}",
"gists_url": "https://api.github.com/users/hub4j-test-org/gists{/gist_id}",
"starred_url": "https://api.github.com/users/hub4j-test-org/starred{/owner}{/repo}",
"subscriptions_url": "https://api.github.com/users/hub4j-test-org/subscriptions",
"organizations_url": "https://api.github.com/users/hub4j-test-org/orgs",
"repos_url": "https://api.github.com/users/hub4j-test-org/repos",
"events_url": "https://api.github.com/users/hub4j-test-org/events{/privacy}",
"received_events_url": "https://api.github.com/users/hub4j-test-org/received_events",
"type": "Organization",
"site_admin": false
},
"html_url": "https://github.com/hub4j-test-org/temp-testChecksWithAppIds",
"description": "A test repository for testing the github-api project: temp-testChecksWithAppIds",
"fork": false,
"url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds",
"forks_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/forks",
"keys_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/keys{/key_id}",
"collaborators_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/collaborators{/collaborator}",
"teams_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/teams",
"hooks_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/hooks",
"issue_events_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/issues/events{/number}",
"events_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/events",
"assignees_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/assignees{/user}",
"branches_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/branches{/branch}",
"tags_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/tags",
"blobs_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/git/blobs{/sha}",
"git_tags_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/git/tags{/sha}",
"git_refs_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/git/refs{/sha}",
"trees_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/git/trees{/sha}",
"statuses_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/statuses/{sha}",
"languages_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/languages",
"stargazers_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/stargazers",
"contributors_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/contributors",
"subscribers_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/subscribers",
"subscription_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/subscription",
"commits_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/commits{/sha}",
"git_commits_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/git/commits{/sha}",
"comments_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/comments{/number}",
"issue_comment_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/issues/comments{/number}",
"contents_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/contents/{+path}",
"compare_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/compare/{base}...{head}",
"merges_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/merges",
"archive_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/{archive_format}{/ref}",
"downloads_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/downloads",
"issues_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/issues{/number}",
"pulls_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/pulls{/number}",
"milestones_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/milestones{/number}",
"notifications_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/notifications{?since,all,participating}",
"labels_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/labels{/name}",
"releases_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/releases{/id}",
"deployments_url": "https://api.github.com/repos/hub4j-test-org/temp-testChecksWithAppIds/deployments",
"created_at": "2024-05-17T13:51:56Z",
"updated_at": "2024-05-17T13:52:00Z",
"pushed_at": "2024-05-17T13:51:57Z",
"git_url": "git://github.com/hub4j-test-org/temp-testChecksWithAppIds.git",
"ssh_url": "git@github.com:hub4j-test-org/temp-testChecksWithAppIds.git",
"clone_url": "https://github.com/hub4j-test-org/temp-testChecksWithAppIds.git",
"svn_url": "https://github.com/hub4j-test-org/temp-testChecksWithAppIds",
"homepage": "http://github-api.kohsuke.org/",
"size": 0,
"stargazers_count": 0,
"watchers_count": 0,
"language": null,
"has_issues": true,
"has_projects": true,
"has_downloads": true,
"has_wiki": true,
"has_pages": false,
"has_discussions": false,
"forks_count": 0,
"mirror_url": null,
"archived": false,
"disabled": false,
"open_issues_count": 0,
"license": null,
"allow_forking": true,
"is_template": false,
"web_commit_signoff_required": false,
"topics": [],
"visibility": "public",
"forks": 0,
"open_issues": 0,
"watchers": 0,
"default_branch": "main",
"permissions": {
"admin": true,
"maintain": true,
"push": true,
"triage": true,
"pull": true
},
"temp_clone_token": "",
"allow_squash_merge": true,
"allow_merge_commit": true,
"allow_rebase_merge": true,
"allow_auto_merge": false,
"delete_branch_on_merge": false,
"allow_update_branch": false,
"use_squash_pr_title_as_default": false,
"squash_merge_commit_message": "COMMIT_MESSAGES",
"squash_merge_commit_title": "COMMIT_OR_PR_TITLE",
"merge_commit_message": "PR_TITLE",
"merge_commit_title": "MERGE_MESSAGE",
"custom_properties": {},
"organization": {
"login": "hub4j-test-org",
"id": 7544739,
"node_id": "MDEyOk9yZ2FuaXphdGlvbjc1NDQ3Mzk=",
"avatar_url": "https://avatars.githubusercontent.com/u/7544739?v=4",
"gravatar_id": "",
"url": "https://api.github.com/users/hub4j-test-org",
"html_url": "https://github.com/hub4j-test-org",
"followers_url": "https://api.github.com/users/hub4j-test-org/followers",
"following_url": "https://api.github.com/users/hub4j-test-org/following{/other_user}",
"gists_url": "https://api.github.com/users/hub4j-test-org/gists{/gist_id}",
"starred_url": "https://api.github.com/users/hub4j-test-org/starred{/owner}{/repo}",
"subscriptions_url": "https://api.github.com/users/hub4j-test-org/subscriptions",
"organizations_url": "https://api.github.com/users/hub4j-test-org/orgs",
"repos_url": "https://api.github.com/users/hub4j-test-org/repos",
"events_url": "https://api.github.com/users/hub4j-test-org/events{/privacy}",
"received_events_url": "https://api.github.com/users/hub4j-test-org/received_events",
"type": "Organization",
"site_admin": false
},
"security_and_analysis": {
"secret_scanning": {
"status": "disabled"
},
"secret_scanning_push_protection": {
"status": "disabled"
},
"dependabot_security_updates": {
"status": "disabled"
},
"secret_scanning_validity_checks": {
"status": "disabled"
}
},
"network_count": 0,
"subscribers_count": 22
}
Loading