-
Notifications
You must be signed in to change notification settings - Fork 7
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
Sftp private key #12
base: develop
Are you sure you want to change the base?
Sftp private key #12
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
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 only reviewed the first half of the PR. Please fix all the files following the general principles as described in the comments.
pom.xml
Outdated
<name>SFTP Actions</name> | ||
<properties> | ||
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | ||
<cdap.version>6.2.0</cdap.version> | ||
<hydrator.version>2.4.0</hydrator.version> | ||
<cdap.version>6.1.2</cdap.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.
Please don't change dependency version on develop branch
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.
versions fixed
pom.xml
Outdated
@@ -34,7 +34,7 @@ | |||
<widgets.dir>widgets</widgets.dir> | |||
<docs.dir>docs</docs.dir> | |||
|
|||
<!-- This is the version range for the app.parents that this plugin is valid for. Usually this will correspond with | |||
<!-- This is the version range for the app.parents that this plugin is valid for. Usally this will correspond with |
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.
Misspell Usually
.
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.
spelling fixed
<dependency> | ||
<groupId>software.sham</groupId> | ||
<artifactId>sham-ssh</artifactId> | ||
<version>0.1.0</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.
Add a <scope>test</scope>
for test only dependency
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.
added
pom.xml
Outdated
<scope>test</scope> | ||
</dependency> | ||
|
||
|
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.
Remove extra new lines
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.
extra lines removed throughout codebase
|
||
@Name("compressionFlag") | ||
@Description("Setting Compression Flag") | ||
public String compressFlag; |
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.
Why this is not optional (@Nullable
)?
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.
@nullable added
@Description("Setting Directory Flag") | ||
public String dirFlag; | ||
|
||
|
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.
Remove extra new lines
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.
Extra Line Removed throughout code
} | ||
|
||
public byte[] getPrivateKey() { | ||
assert privateKey != null; |
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.
Not needed
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.
removed
|
||
public byte[] getPassphrase(){ | ||
if (passphrase == null){ | ||
passphrase = ""; |
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.
Don't mutate field on get call.
return passphrase == null ? new byte[0] : passphrase.getBytes(StandardCharsets.UTF_8);
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.
changed from feedback
if (compressFlag.equals("compression-off")){ | ||
return compressFlag = ""; | ||
} | ||
return compressFlag="-C"; |
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.
Don't mutate field on get call.
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.
changed from feedback
Changed formatting
Added PrivateKey SSH functionality and new SCP Remote to Remote Action