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 fork branch sync #1898

Merged
merged 6 commits into from
Aug 5, 2024
Merged

Add fork branch sync #1898

merged 6 commits into from
Aug 5, 2024

Conversation

jonesbusy
Copy link
Contributor

@jonesbusy jonesbusy commented Jul 17, 2024

Description

Add support for fork sync branch

Fixing #1626

Added two wiremock test and tested a SNAPSHOT version in a WIP PR jenkinsci/plugin-modernizer-tool#141

I confirm this work by enabling logs and ensuring my fork is sync directly on GitHub

(e24ddd0) GitHub API request: POST https://api.github.com/repos/jonesbusy/jobcacher-plugin/merge-upstream 

Before submitting a PR:

  • Changes must not break binary backwards compatibility. If you are unclear on how to make the change you think is needed while maintaining backward compatibility, CONTRIBUTING.md for details.
  • Add JavaDocs and other comments explaining the behavior.
  • When adding or updating methods that fetch entities, add @link JavaDoc entries to the relevant documentation on https://docs.github.com/en/rest .
  • Add tests that cover any added or changed code. This generally requires capturing snapshot test data. See CONTRIBUTING.md for details.
  • Run mvn -D enable-ci clean install site locally. If this command doesn't succeed, your change will not pass CI.
  • Push your changes to a branch other than main. You will create your PR from that branch.

When creating a PR:

  • Fill in the "Description" above with clear summary of the changes. This includes:
  • If this PR fixes one or more issues, include "Fixes #" lines for each issue.
  • Provide links to relevant documentation on https://docs.github.com/en/rest where possible. If not including links, explain why not.
  • All lines of new code should be covered by tests as reported by code coverage. Any lines that are not covered must have PR comments explaining why they cannot be covered. For example, "Reaching this particular exception is hard and is not a particular common scenario."
  • Enable "Allow edits from maintainers".

API is https://docs.github.com/en/rest/branches/branches?apiVersion=2022-11-28#sync-a-fork-branch-with-the-upstream-repository

Code is covered 100%

Screenshot from 2024-07-24 12-38-45

Copy link

codecov bot commented Jul 17, 2024

Codecov Report

Attention: Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.

Project coverage is 81.32%. Comparing base (3c4d80c) to head (6493b2c).

Files Patch % Lines
src/main/java/org/kohsuke/github/GHBranchSync.java 87.50% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1898      +/-   ##
============================================
+ Coverage     81.29%   81.32%   +0.02%     
- Complexity     2450     2457       +7     
============================================
  Files           238      239       +1     
  Lines          7449     7463      +14     
  Branches        400      400              
============================================
+ Hits           6056     6069      +13     
- Misses         1146     1147       +1     
  Partials        247      247              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

Second rounds of feedback, sorry. This is looking good, just a little more to be done.
Thanks!

* @throws IOException
* the io exception
*/
public GHRepository sync(String branch) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

The call actually has a return record that could be useful. Example from the docs:

{
  "message": "Successfully fetched and fast-forwarded from upstream defunkt:main",
  "merge_type": "fast-forward",
  "base_branch": "defunkt:main"
}

Check the schema and return a matching record.

@Test
public void sync() throws IOException {
GHRepository r = getRepository();
r.sync("main");
Copy link
Member

Choose a reason for hiding this comment

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

Add slightly more complex tests to cover the returned record and also the error/exception case. (You can do all in one method if you want or separately.)

@jonesbusy
Copy link
Contributor Author

Sure thanks, I will try to add a new model for the sync response and perform some assertion on it

@jonesbusy
Copy link
Contributor Author

I've added the GHBranchSync model and return it

I've also added a negative test when the API return a non-200 status code (for example 422 when the repo is not a fork)

Copy link

@gounthar gounthar left a comment

Choose a reason for hiding this comment

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

Nice, thanks! 👍

Copy link

@NotMyFault NotMyFault left a comment

Choose a reason for hiding this comment

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

Awesome work!

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.

Great work on this. Thanks!

Comment on lines +75 to +76
GHBranchSync sync = r.sync("main");
fail("Should have thrown an exception");
Copy link
Member

Choose a reason for hiding this comment

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

There's an assertThrows() method that I prefer for this, but I'm going to block merge for it.

src/main/java/org/kohsuke/github/GHBranchSync.java Outdated Show resolved Hide resolved
@jonesbusy
Copy link
Contributor Author

Apparently something wrong with javadoc. I will take a look

@jonesbusy
Copy link
Contributor Author

Should be fixed, can you restart the workflows please ?

@jonesbusy
Copy link
Contributor Author

Hi @bitwiseman Sorry for the ping, but where you able to review again this PR ?

It's also update with the default branch

Thanks!

@bitwiseman bitwiseman merged commit af5bde7 into hub4j:main Aug 5, 2024
12 checks passed
@jonesbusy jonesbusy deleted the feature/fork-sync branch August 5, 2024 16:54
@jonesbusy
Copy link
Contributor Author

Thanks for the merge @bitwiseman. Sorry to ping you directly (again) but is there a release soon including this feature ?

I'm asking because we are approaching the end of the Google Summer of Code on our project https://github.com/jenkinsci/plugin-modernizer-tool and would be great that we can include the new release on our delivery

The fork sync is quiet critical on our project to avoid opening PRs from outdated forks.

Thanks for your consideration 😃

@bitwiseman
Copy link
Member

I'll do a release today.

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.

4 participants