Skip to content

Conversation

@JinwooHwang
Copy link
Contributor

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 build run 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?

@raboof
Copy link
Member

raboof commented Aug 16, 2025

It would be good to mention the motivation for the change in the PR.

In this case indeed NULL_OUTPUT_STREAM has been deprecated in favour of INSTANCE (https://commons.apache.org/proper/commons-io/apidocs/org/apache/commons/io/output/NullOutputStream.html)

@JinwooHwang JinwooHwang requested a review from raboof August 27, 2025 09:57
@raboof raboof mentioned this pull request Aug 27, 2025
6 tasks
@raboof
Copy link
Member

raboof commented Aug 27, 2025

It would be good to mention the motivation for the change in the PR.

In this case indeed NULL_OUTPUT_STREAM has been deprecated in favour of INSTANCE (https://commons.apache.org/proper/commons-io/apidocs/org/apache/commons/io/output/NullOutputStream.html)

OK looks like the motivation is just "I wanted to update commons-io and had to fix this deprecation error" - simple, but still would be worth mentioning :)

@JinwooHwang
Copy link
Contributor Author

Yes, that’s exactly it, @raboof. I just wanted to update commons-io and needed to address the deprecation error along the way. Thank you for pointing out that it’s worth mentioning — I really appreciate the reminder, I’ll be sure to note it more clearly next time.

Copy link
Member

@raboof raboof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that the main branch is green after #7916 I'm confident any regressions caused by the update will be noticed, so this LGTM.

@raboof raboof merged commit 8e0fdc2 into apache:develop Aug 28, 2025
50 of 65 checks passed
@raboof
Copy link
Member

raboof commented Aug 28, 2025

@JinwooHwang-SAS this caused a failure of the integration tests in https://github.com/apache/geode/actions/runs/17291015222/job/49079826921 - looks like the version update needs to be registered somewhere else as well? Could you follow up?

@JinwooHwang
Copy link
Contributor Author

I have addressed the issue via 7ee1fbe, @raboof . Thank you.

@raboof
Copy link
Member

raboof commented Aug 28, 2025

cool - is that commit in a PR somewhere yet? (also the comment seems to mention beanutils instead of io)

@JinwooHwang
Copy link
Contributor Author

@raboof . Yes, I made the update while working on PR #7904. In hindsight, it would have been much clearer to include it in PR #7902, and I sincerely apologize for that oversight. Thank you very much for catching this and for your patience — I truly appreciate your guidance and support throughout the review.

JinwooHwang added a commit that referenced this pull request Sep 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants