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

GH-3211: Add DefSftpSF.setKnownHosts(Resource) #3212

Merged
merged 2 commits into from
Mar 10, 2020

Conversation

artembilan
Copy link
Member

Fixes #3211

  • Add DefaultSftpSessionFactory.setKnownHosts(Resource) to
    allow to configure externally any resource for file with known_hosts content
  • Deprecate an existing method in favor of new one
  • The new method makes it aligned with the setPrivateKey(Resource)
  • Fix tests do not use a deprecated method any more

Cherry-pick to 5.2.x

Fixes spring-projects#3211

* Add `DefaultSftpSessionFactory.setKnownHosts(Resource)` to
allow to configure externally any resource for file with known_hosts content
* Deprecate an existing method in favor of new one
* The new method makes it aligned with the `setPrivateKey(Resource)`
* Fix tests do not use a deprecated method any more

**Cherry-pick to 5.2.x**
* @see JSch#setKnownHosts(InputStream)
* @since 5.2.5
*/
public void setKnownHosts(Resource knownHosts) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this cause a problem with <bean/> config? (ambiguous setter) setKnownHostsResource() ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like Juergen has fixed something in SF. See Travis 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

If I recall correctly, failures were random; also it looks like you changed all the tests to return a Resource using SpEL.

Perhaps the problem only occurs when a PropertyEditor is needed (although I remember most of the problems were with String Vs. Expression .

I will defer to you, though, if you think it's safe.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I can rename it to setKnownHostsResource to have it really very safe...

* Change `sftp.adoc` to reflect a new property
*/
@Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we really deprecate? This is easier if you are using Java Config and want to point to a simple file name.

Not related to this PR but perhaps we should consider adding a SftpSessionFactorySpec ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Then we need also to add a string-based for privateKey.

More over it is better to resolve any resources via @Value Resource then inject.

@garyrussell garyrussell merged commit 6b82681 into spring-projects:master Mar 10, 2020
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.

DefaultSftpSessionFactory file resolving inconsistency
2 participants