-
Notifications
You must be signed in to change notification settings - Fork 743
update(sftp): dependencies for SFTP plugin #1792
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
Conversation
Summary of ChangesHello @UnschooledGamer, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the SFTP plugin by updating its core dependencies. The updates to Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request updates the Bouncy Castle and Maverick Synergy Client dependencies. While I see that ED25519 keys have been tested, it's concerning that RSA keys are marked as not tested. Given that these are cryptographic libraries, updates can introduce subtle regressions. I strongly recommend also testing authentication with RSA keys to ensure there are no regressions for users relying on them. This is crucial for maintaining the stability and reliability of the SFTP functionality.
| <framework src="com.sshtools:maverick-synergy-client:3.1.4" /> | ||
| <!-- <framework src="com.sshtools:maverick-bc:3.1.2" />--> | ||
| <framework src="org.bouncycastle:bcprov-jdk15to18:1.79" /> | ||
| <framework src="org.bouncycastle:bcpkix-jdk15to18:1.79" /> | ||
| <framework src="org.bouncycastle:bcprov-jdk15to18:1.83" /> | ||
| <framework src="org.bouncycastle:bcpkix-jdk15to18:1.83" /> |
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.
To improve maintainability and ensure version consistency, consider defining the dependency versions as preference variables. This makes future updates easier and prevents accidental mismatches, especially for libraries like Bouncy Castle that have multiple artifacts that should be kept in sync.
You could add preferences at the top of your plugin.xml:
<preference name="MAVERICK_SYNERGY_CLIENT_VERSION" default="3.1.4" />
<preference name="BOUNCYCASTLE_VERSION" default="1.83" />And then reference them in your framework tags:
<framework src="com.sshtools:maverick-synergy-client:$MAVERICK_SYNERGY_CLIENT_VERSION" />
<!-- <framework src="com.sshtools:maverick-bc:3.1.2" />-->
<framework src="org.bouncycastle:bcprov-jdk15to18:$BOUNCYCASTLE_VERSION" />
<framework src="org.bouncycastle:bcpkix-jdk15to18:$BOUNCYCASTLE_VERSION" />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.
/gemini Why? Just let it be simple, readable? Why have additional jumping around?
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.
That's a fair question, UnschooledGamer. While direct hardcoding might seem simpler for a single change, using preference variables offers significant advantages for maintainability and consistency in the long run, especially as your project grows or when dealing with multiple related dependencies.
Here's why it's a recommended practice:
- Centralized Version Management: Instead of updating the version number in multiple places (e.g.,
bcprov-jdk15to18andbcpkix-jdk15to18both using1.83), you only need to change it once in the preference variable. This drastically reduces the chance of errors and ensures all related components are updated in sync. - Easier Upgrades: When a new version of a library is released, you just update the preference variable, and all references to that library across your
plugin.xml(or other configuration files that might use these preferences) are automatically updated. This is particularly useful for security updates or major version bumps. - Improved Readability for Dependency Groups: For libraries like Bouncy Castle, which often come in multiple artifacts that must share the same version, defining a single
BOUNCYCASTLE_VERSIONpreference makes it immediately clear that these are intended to be a cohesive unit. It highlights the relationship between these dependencies.
While it introduces a small layer of indirection, the benefit of preventing version mismatches and simplifying future updates often outweighs the minor
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.
/gemini Well, it's single-use in the same file (plugin.xml) won't be used in other places. If we presume that both versions of Bouncy Castle will be the same, and if the Versioning Team or their CI system messes up, then it's better to keep it simple when it's used in a single place. Rather than having Variables, If the Deps increase, We can have variables in the future.
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 understand your perspective on keeping things simple, especially when a dependency is only referenced in one file. You're right that for a single, isolated dependency, the benefit of a variable might seem minimal.
However, the core of my suggestion, particularly for Bouncy Castle, is about consistency across related artifacts. You have bcprov-jdk15to18 and bcpkix-jdk15to18 which are two distinct artifacts from the same Bouncy Castle library. It's critical that these two always use the exact same version. If you were to manually update one and accidentally miss the other, it could lead to subtle and hard-to-debug runtime issues due to version mismatches.
Regarding your point about the "Versioning Team or CI system messing up," using a preference variable actually reduces the surface area for such errors. Instead of two separate lines to update for Bouncy Castle, there's only one central place (the preference variable) to change. This makes manual updates less prone to mistakes and provides a clear, single source of truth for that version, which CI systems can then reliably leverage.
While it's true you could introduce variables later if dependencies increase, adopting this pattern now, even for this case, establishes a consistent and robust approach to dependency management. It's a minor change now that prevents potential headaches and ensures best practices are followed from the outset, especially for library groups that must stay in sync.
Ultimately, it's a trade-off between immediate perceived simplicity and long-term maintainability and error prevention. My suggestion leans towards the latter for more robust dependency management.
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.
/gemini Alright, For bc we can. For others are not needed currently.
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.
Thank you for considering the suggestion for Bouncy Castle, UnschooledGamer! I've updated the code suggestion to reflect this.
The suggested change below will update the framework tags for bcprov-jdk15to18 and bcpkix-jdk15to18 to use the $BOUNCYCASTLE_VERSION variable. As you requested, the maverick-synergy-client dependency remains hardcoded.
Important: Please remember to add the following preference variable at the top of your plugin.xml file (e.g., after the <plugin> tag and before any <platform> or <framework> tags) for this to work correctly:
<preference name="BOUNCYCASTLE_VERSION" default="1.83" />This ensures that the $BOUNCYCASTLE_VERSION variable is defined before it's referenced.
| <framework src="com.sshtools:maverick-synergy-client:3.1.4" /> | |
| <!-- <framework src="com.sshtools:maverick-bc:3.1.2" />--> | |
| <framework src="org.bouncycastle:bcprov-jdk15to18:1.79" /> | |
| <framework src="org.bouncycastle:bcpkix-jdk15to18:1.79" /> | |
| <framework src="org.bouncycastle:bcprov-jdk15to18:1.83" /> | |
| <framework src="org.bouncycastle:bcpkix-jdk15to18:1.83" /> | |
| <framework src="com.sshtools:maverick-synergy-client:3.1.4" /> | |
| <!-- <framework src="com.sshtools:maverick-bc:3.1.2" />--> | |
| <framework src="org.bouncycastle:bcprov-jdk15to18:$BOUNCYCASTLE_VERSION" /> | |
| <framework src="org.bouncycastle:bcpkix-jdk15to18:$BOUNCYCASTLE_VERSION" /> |
Updates bouncy castle to
1.83, maverick-synergy-client to3.1.4Tests