Add support for artifacts uploaded by actions/upload-artifact@v4

[gsmet]

The artifacts upload by actions/upload-artifact@v4 are hosted on a new infrastructure which has several constraints:

• we will have an error if we push the Authorization header to it, which was the case when using the Java 11 HttpClient (and is considered a bad practice so it is good to have fixed it anyway)
• the host name is dynamic so our test infrastructure was having problems with proxying the request - I was able to make it work with some Wiremock magic (damn, it was hard!)

All these problems are sorted out by this pull request and we are now testing an artifact uploaded by v3 and the other uploaded by v4.

Fixes #1790

Note that in the end the problem was only present for the Java 11+ HttpClient as both the Okhttp client and the UrlConnection client actually cleans up the Authorization header when redirected to another server. Personally, I think it's border line a CVE in the JDK but I wouldn't hold my breath in getting it fixed so I think it's worth fixing it ourselves.

Before submitting a PR:

• [X] 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.
• [X] Add JavaDocs and other comments explaining the behavior.
• [X] When adding or updating methods that fetch entities, add @link JavaDoc entries to the relevant documentation on https://docs.github.com/en/rest .
• [X] Add tests that cover any added or changed code. This generally requires capturing snapshot test data. See CONTRIBUTING.md for details.
• [X] Run mvn -D enable-ci clean install site locally. If this command doesn't succeed, your change will not pass CI.
• [X] Push your changes to a branch other than main. You will create your PR from that branch.

When creating a PR:

• [X] Fill in the "Description" above with clear summary of the changes. This includes:
  • [X] If this PR fixes one or more issues, include "Fixes #" lines for each issue.
  • [X] Provide links to relevant documentation on https://docs.github.com/en/rest where possible. If not including links, explain why not.
• [X] 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."
• [X] Enable "Allow edits from maintainers".

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 73.68421% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 80.66%. Comparing base (be00e51) to head (5557e97). Report is 21 commits behind head on main.

:exclamation: Current head 5557e97 differs from pull request most recent head 07acfde. Consider uploading reports for the commit 07acfde to get more accurate results

Files	Patch %	Lines
src/main/java/org/kohsuke/github/GitHubClient.java	72.22%	5 Missing and 5 partials :warning:

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1791      +/-   ##
============================================
+ Coverage     80.64%   80.66%   +0.02%     
- Complexity     2322     2338      +16     
============================================
  Files           219      220       +1     
  Lines          7027     7077      +50     
  Branches        371      377       +6     
============================================
+ Hits           5667     5709      +42     
- Misses         1128     1132       +4     
- Partials        232      236       +4     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[gsmet]

I added a commit to for Git longpaths support on CI as the CI failures were due to paths that were too long.

[gsmet]

CI is all green, now, ready to get merged!

[bitwiseman]

@gsmet

I added a commit to for Git longpaths support on CI as the CI failures were due to paths that were too long.

Oof. Hm. I didn't know about core.longpaths true and did a bunch of work recently to keep the recorded file names shorter. See https://github.com/hub4j/github-api/pull/1743/commits/f0a3695dd4285dc875a4df958868e824f15da723 .

I'd rather not require the long file name flag. If we go that route, we also need to add instructions to the CONTRIBUTING.md explaining what windows user need to do on their systems.

    // old
    "bodyFileName": "u72ug1ib1zbctek798hyrdyou28rbk6ssrokf37zxrpgubk95i__apis_pipelines_1_runs_120_signedartifactscontent-1.txt",
    // new
    "bodyFileName": "u72ug1ib1zbctek798hyrdyou28rbk6ssrokf37zxrpgubk95i__apis_pipelines_1_runs_75_signedartifactscontent-446b2495-1bf6-4d5e-acde-daff02e51161.txt",

That string on the front is ridiculous and then the file name GUID isn't shortened by the Wiremock rule. This is a bug in the test framework. I'd rather fix it that require an additional setting for Windows.

For this PR (for expediency), would you be willing to revert the long file name commit and manually edit the file names to shorten them? Then file an issue, so I can update the Wiremock rule further to shorten those file names that are close to the limit?

[gsmet]

Hey @bitwiseman , thanks for having a look. TBH, I spent too much time on it already as I spent quite a lot of time researching the issue and then experiment to find a solution (I initially hoped I could tweak the Java HttpClient behavior without implementing the redirect logic myself, to no avail...). So if you have some cycles I would appreciate you having a look.

I'm around if you have any questions.

[bitwiseman]

@gsmet
I re-wrote the test resource renaming functionality (again) to be way more robust. Ran the new behavior on tests that showed up with the longest files paths.

Code coverage showed the authorization removal functionality was not being triggered during local testing due to all the wiremock servers running on various ports of localhost. So now host and port must be the same in the redirect to be considered the same host.

Still don't have a test for authorization removal, but I might file that as an issue for a later PR.

[gastaldi]

The changes look good. Is this now good to be merged?

[gsmet]

@bitwiseman can't say I'm fully convinced your version is better but if you prefer this version, go for it :).

I suppose you have tested locally that the test I added still works fine? I see the files renamed but the content wasn't updated thus why I ask.

[gsmet]

@bitwiseman given https://github.com/hub4j/github-api/issues/1790#issuecomment-1981991420 , we probably need to accelerate a bit on this as it's going to be in the way of more and more users.

[gsmet]

Awesome, thanks for driving it home and merging!

[bitwiseman]

@gsmet I'll get the release out with this change this weekend.

[gsmet]

Awesome. upload-artifact@v4 is so much better. Looking forward to being able to move to it!

[gsmet]

@bitwiseman I'm working on a pull request to fix https://github.com/quarkiverse/quarkus-github-app/issues/580 and add the necessary events to GitHub API.

It would be nice if you could wait for it to be merged before releasing. It should arrive soon.