Skip to content
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

DLPX-88137 Upgrade sshj with Raul's mod dlpx73623 #7

Conversation

lschetanrao
Copy link

@lschetanrao lschetanrao commented Oct 9, 2023

JIRA : https://delphix.atlassian.net/browse/DLPX-88137

Problem

Updating sshj with custom changes.

Solution

This PR contains following changes

  1. Clean cherry-pick of DLPX-84042 Upgrade sshj to latest version (plus Raul's mods) #6 on top of the latest release, 0.36.0.
  2. Fixed unit test cases by replacing junit with jupiter junit because SSHJ has removed junit dependency. Please check this commit af05750

Published the JAR on artifactory as version 0.36.0-dlpx73623. https://artifactory.delphix.com/ui/repos/tree/General/appstack-internal-services/com/hierynomus/sshj/0.36.0-dlpx73623/sshj-0.36.0-dlpx73623.jar

Published the POM file by copying the pom from https://repo1.maven.org/maven2/com/hierynomus/sshj/0.36.0/sshj-0.36.0.pom and replacing the version from 0.36.0 to 0.36.0-dlpx73623

@lschetanrao lschetanrao force-pushed the dlpx/pr/lschetanrao/89a91921-9c77-4058-8350-1bde7552b698 branch from 0e7617b to 6a2ae6e Compare October 9, 2023 09:29
@codecov-commenter
Copy link

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (f4d34d8) 68.46% compared to head (af05750) 68.56%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@                    Coverage Diff                     @@
##             DLPX-88137-on-0.36.0       #7      +/-   ##
==========================================================
+ Coverage                   68.46%   68.56%   +0.10%     
- Complexity                   1398     1417      +19     
==========================================================
  Files                         207      208       +1     
  Lines                        7423     7492      +69     
  Branches                      629      638       +9     
==========================================================
+ Hits                         5082     5137      +55     
- Misses                       2009     2021      +12     
- Partials                      332      334       +2     
Files Coverage Δ
...hmizz/sshj/connection/channel/AbstractChannel.java 74.69% <ø> (ø)
...sshj/connection/channel/direct/SessionChannel.java 50.00% <ø> (ø)
src/main/java/net/schmizz/sshj/ConfigImpl.java 84.61% <50.00%> (-2.27%) ⬇️
...n/java/net/schmizz/sshj/common/CircularBuffer.java 88.40% <88.40%> (ø)
...zz/sshj/connection/channel/ChannelInputStream.java 79.03% <42.85%> (+0.24%) ⬆️

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lschetanrao lschetanrao self-assigned this Oct 10, 2023
@lschetanrao lschetanrao marked this pull request as ready for review October 10, 2023 09:05
@sebroy
Copy link

sebroy commented Oct 10, 2023

@rasantel @lschetanrao Is there any hope of upstreaming our fix so that we don't have to maintain this fork?

Copy link

@VenkatanadhanG VenkatanadhanG left a comment

Choose a reason for hiding this comment

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

Waiting for ab pre push and dx test with new version of sshj and manual testing to confirmation on app-gate.

@rasantel
Copy link

rasantel commented Oct 10, 2023

@rasantel @lschetanrao Is there any hope of upstreaming our fix so that we don't have to maintain this fork?

@sebroy Our fix converts the buffer class to a circular buffer only for the parts that we use, so there are now two classes - the old one used in other parts of sshj and the new one. I was hoping to find time to complete the conversion and get rid of the old buffer before submitting, but in practice there's always something else taking my time. Finishing the conversion is not necessarily trivial because of the impact throughout the entire library. I suppose I could submit the partial conversion and see what happens.

@rasantel
Copy link

@sebroy By the way, I already upstreamed another race condition fix, hierynomus#851, which allowed us to be able to upgrade sshj again.

@sebroy
Copy link

sebroy commented Oct 11, 2023

@sebroy By the way, I already upstreamed another race condition fix, hierynomus#851, which allowed us to be able to upgrade sshj again.

@rasantel that's excellnt, thank you.

@VenkatanadhanG VenkatanadhanG self-requested a review October 16, 2023 04:25
@lschetanrao lschetanrao merged commit 55e3225 into DLPX-88137-on-0.36.0 Oct 16, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants