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

user, push, pull event extensions #944

Merged
merged 4 commits into from
Nov 3, 2020

Conversation

seregamorph
Copy link
Contributor

@seregamorph seregamorph commented Sep 24, 2020

Description

  • GitHub event pull_request (GHEventPayload.PullRequest) new field label of type GHLabel (may present when "action":"labeled"); example json with test provided
  • GitHub event push (GHEventPayload.Push) new field compare
  • GitUser new field username (e.g. push.commits[*].author.username)

Before submitting a PR:

We love getting PRs, but we hate asking people for the same basic changes every time.

  • Push your changes to a branch other than master. Create your PR from that branch.
  • Add JavaDocs and other comments
  • Write tests that run and pass in CI. See CONTRIBUTING.md for details on how to capture snapshot data.
  • Run mvn clean compile locally. This may reformat your code, commit those changes.
  • Run mvn -D enable-ci clean install site locally. If this command doesn't succeed, your change will not pass CI.

When creating a PR:

  • Fill in the "Description" above.
  • Enable "Allow edits from maintainers".

@bitwiseman
Copy link
Member

@seregamorph
Maybe you could split out the changes change into a separate PR? We could do the obvious modifications first.

* @return GitHub username
*/
public String getUsername() {
return username;
Copy link
Member

Choose a reason for hiding this comment

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

I don't see this field in the git user information (https://docs.github.com/en/rest/reference/git#get-a-commit). Where are you finding this?

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I see it here:

but it isn't in the current GitHub docs. Might be only present in older versions of GitHub Server?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have it in GitHub (global; not enterprise) standard webhook notification on push event

Copy link
Member

@bitwiseman bitwiseman Sep 24, 2020

Choose a reason for hiding this comment

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

@seregamorph
Interesting. Is it possible it is only present on events?
This doesn't include it: https://api.github.com/repos/octocat/Hello-World/git/commits/7fd1a60b01f91b314f59955a4e4d4e80d8edf11d

I'd be okay with adding this if the javadoc clearly states that is only seen on events and we add @Preview @Deprecated annotations to it. I would also ask that you contact GitHub support and ask them to clarify the documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Marked as

    @Preview
    @Deprecated
    @CheckForNull

and added javadoc

Copy link

Choose a reason for hiding this comment

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

* @return compare
*/
public String getCompare() {
return compare;
Copy link
Member

Choose a reason for hiding this comment

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

https://docs.github.com/en/developers/webhooks-and-events/webhook-events-and-payloads#push
Has compare in the example but not in the documented fields. Not sure which is in error. 😢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can check the Webhook payload example on the link that you gave, the field is here (but not listed in description, right)

@seregamorph seregamorph force-pushed the feature/push-pulls-extensions branch from 0d92f76 to b7d03f7 Compare September 24, 2020 19:46
@seregamorph
Copy link
Contributor Author

changes related part was erased (force pushed) from the scope

Copy link
Member

@bitwiseman bitwiseman left a comment

Choose a reason for hiding this comment

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

Still a few questions left to resolve. Thanks for working on this.

src/main/java/org/kohsuke/github/GitUser.java Outdated Show resolved Hide resolved
src/main/java/org/kohsuke/github/GitUser.java Outdated Show resolved Hide resolved
@seregamorph seregamorph marked this pull request as ready for review September 24, 2020 19:59
@bitwiseman
Copy link
Member

@seregamorph
Did see this? https://github.com/hub4j/github-api/pull/944/files#r494672997
You can change it, we can discuss further in the PR, or you can split that part of the change into another PR and we can continue discussion there. Up to you. Thanks for contributing!

@seregamorph
Copy link
Contributor Author

@seregamorph
Did see this? https://github.com/hub4j/github-api/pull/944/files#r494672997
You can change it, we can discuss further in the PR, or you can split that part of the change into another PR and we can continue discussion there. Up to you. Thanks for contributing!

Yes. I raised a question in https://github.community, but it still waits for moderation.
Screenshot 2020-09-26 at 11 55 15

So let's wait a bit.

@seregamorph
Copy link
Contributor Author

GitHub community topic is still under review and is not published (already 3 days). Anything else I can do here now?

@bitwiseman bitwiseman merged commit d4c5c6a into hub4j:master Nov 3, 2020
@seregamorph seregamorph deleted the feature/push-pulls-extensions branch November 5, 2020 08:50
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