-
Notifications
You must be signed in to change notification settings - Fork 695
commons-logging 1.3.5 #7903
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
commons-logging 1.3.5 #7903
Conversation
raboof
left a comment
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.
I'm generally in favor of this change, but I'm reluctant to approve it because the 'develop / integrationTest' and 'develop / acceptanceTest' tests are failing.
The fact that those tests are failing is not your fault: they are already failing on master. These failures are tracked in the bug tracker as https://issues.apache.org/jira/browse/GEODE-10458 and https://issues.apache.org/jira/browse/GEODE-10459 .
If after analysis it appears that it is too complicated to fix those tests, I suppose we could disable those specific tests (while ideally keeping any other tests in those jobs) and leave the issues open to revisit re-enabling them again later. Would you have a chance to look into that?
|
Thank you for your feedback, @raboof. Could you please take a look at 9d736a9 to see if it takes care of https://issues.apache.org/jira/browse/GEODE-10458 ? |
When you open that commit page, near the top you see "Commit 9d736a9, JinwooHwang-SAS authored 16 hours ago . x 14 / 16 . Verified". If you click the If you click on 'Details' next to I see It would be super cool if we could also make |
|
Thank you @raboof for the thoughtful follow-up and for updating the PR title to include GEODE-10458—much appreciated. I'm glad to hear the integration test is now passing and that the fix is confirmed. I agree it would have been ideal to reference the JIRA ticket directly in the commit; I’ll be more mindful of that going forward. Regarding the acceptance test, could you please let me know how to access the build machine to investigate whether Local Docker Compose is on the PATH? That would help me to assess feasibility more accurately. Are you more comfortable with approving the merge at this point? |
The CI is running on GitHub Actions, so there is no access to the build machine, it is completely driven by
Now that the main branch is green after #7916 I'm confident any regressions caused by the update will be noticed, so I'll merge this. |
|
Thank you very much @raboof |
(cherry picked from commit 785f80a)
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
Has your PR been rebased against the latest commit within the target branch (typically
develop)?Is your initial contribution a single, squashed commit?
Does
gradlew buildrun cleanly?Have you written or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?