-
-
Notifications
You must be signed in to change notification settings - Fork 314
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 validation for platform strings to exclude_parser.py #5651
Conversation
@smlambert Would appreciate your thoughts here on the approach before I exit Draft. And maybe I need to update the GH actions somewhere to fail when this exits with code 1? |
Alternative of global ERROR_FOUND ( needed for all log.error) to fail the github check is adding custom handler.
|
This change now:
Due to how the code is structured, an error is logged whenever an exception is raised. This will now mean any of those cases will cause the process to exit with code 1. This is overall an improvement as it now covers validating the format of the entire exclude string:
|
This is a breaking change, the process will now exit with code 1 when:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very good to me, thanks @jiekang! Have you use a personal repo to do some basic testing (as many of us end up doing with these workflow PRs)?
Based on this change, it detected following errors with openjdk problemlist (current supports jdk version, 8,11,17,21,23,24) ERROR - ./openjdk/excludes/ProblemList_openjdk23.txt:151 : Not exactly 3 elements when splitting java/net/DatagramSocket/GetLocalAddress.java #3088 aix-ppc64, linux-ppc64le ERROR - ./openjdk/excludes/ProblemList_openjdk23.txt:151 : Not exactly 3 elements when splitting java/net/DatagramSocket/GetLocalAddress.java #3088 aix-ppc64, linux-ppc64le @jiekang would you be able to do a following PR to fix those? I believe with this PR in, any PR with changes in openjdk/exclude will also trigger those errors. |
Fixes: #5650