Fix artifact staging windows filename - #39916
Conversation
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
0ce85b9 to
62ee399
Compare
62ee399 to
3a7fd52
Compare
|
R: @Eliaaazzz since you reviewed #39363 |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #39916 +/- ##
============================================
+ Coverage 55.06% 58.34% +3.28%
- Complexity 1722 13485 +11763
============================================
Files 1075 2568 +1493
Lines 171582 268734 +97152
Branches 1262 11029 +9767
============================================
+ Hits 94474 156803 +62329
- Misses 74838 105999 +31161
- Partials 2270 5932 +3662
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Eliaaazzz
left a comment
There was a problem hiding this comment.
Thanks for the ping, Yi, and thanks for picking this up together with the Windows CI wiring. I checked out the head commit on Windows 11 (JDK 11) and ran the module tests locally.
What I verified locally:
:runners:java-fn-execution:testpasses at head, all 138 tests across 34 classes, including the newtestStageArtifactsWithInvalidFilenameCharactersand the reworkedProcessManagerTest.- The new test genuinely catches a regression on Windows: if I revert the sanitization, it fails with
ExecutionException: StatusRuntimeException: UNKNOWNout ofoffer, and the server side logsInvalidPathException: Illegal char <:>. With the windows-latest leg now running this module injava_tests.yml, that failure would show up in CI. - The environment sanitization resolves exactly the failure reported in #39336, and the env id in the new test matches the one from that report.
One substantive comment inline on createFilename, with a repro: the base component can still produce an invalid name for file-type artifacts.
On the two red checks:
beam_PreCommit_Flink_Containertimed out in the Java Combine step. The last logged Gradle task is:runners:flink:2.0:jarand no test execution output appears before the timeout, while:runners:java-fn-execution:jarhad completed cleanly a few minutes earlier. Recent master runs of that workflow are green, and I could not find a patch-related failure in the log.codecov/patchflags three missing or partial lines, and they are theIS_OS_WINDOWSbranches; the Java coverage upload runs on Ubuntu, which cannot take them.
Small question on java_tests.yml: the harness and runner steps used to carry if: always(), so they still ran when :sdks:java:core:test failed. Now an early failure skips them. Is that trade intentional, together with the single !success() upload?
…ing-windows-filename
* Handle invalid chars in base as well * Fix white spaces
5f6cc74 to
daebfe4
Compare
Please add a meaningful description for your change here
Fixes #39336
Port from #39363,
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.