Closed gsmet closed 7 months ago
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: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
I added a commit to for Git longpaths support on CI as the CI failures were due to paths that were too long.
CI is all green, now, ready to get merged!
@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?
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.
@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.
The changes look good. Is this now good to be merged?
@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.
@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.
Awesome, thanks for driving it home and merging!
@gsmet I'll get the release out with this change this weekend.
Awesome. upload-artifact@v4
is so much better. Looking forward to being able to move to it!
@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.
The artifacts upload by actions/upload-artifact@v4 are hosted on a new infrastructure which has several constraints:
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:
@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: