-
Notifications
You must be signed in to change notification settings - Fork 159
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
Merge property file reading logic #769
Conversation
Or-Geva
commented
Dec 4, 2023
- All tests passed. If this feature is not already covered by the tests, I added new tests.
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'd like to review again the tests after fixing them
build-info-extractor/src/test/java/org/jfrog/build/extractor/BuildExtractorUtilsTest.java
Outdated
Show resolved
Hide resolved
@@ -115,33 +111,34 @@ public void getBuildInfoPropertiesFromEncryptedFile() throws IOException, Invali | |||
} | |||
|
|||
public void failToReadEncryptedFileWithNoKey() throws IOException, InvalidAlgorithmParameterException, NoSuchPaddingException, IllegalBlockSizeException, NoSuchAlgorithmException, BadPaddingException, InvalidKeyException { | |||
setupEncryptedFileTest(); | |||
// Create encrypted file with properties | |||
createProperty(); |
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.
This line does nothing. Shouldn't be this?
try (FileOutputStream fileOutputStream = new FileOutputStream(tempFile.toFile())) {
createProperty().store(fileOutputStream, "");
}
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.
You're right, it should be setupEncryptedFileTest(createProperty());
and we are removing the keys on line 117
build-info-extractor/src/test/java/org/jfrog/build/extractor/BuildExtractorUtilsTest.java
Outdated
Show resolved
Hide resolved
return props; | ||
} | ||
|
||
private Properties createPropertyEnv() { |
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.
This function does not create environment variables.
It also does the same as createProperty.
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.
It generates slightly different properties apart from using createProperty to specifically handle properties associated with environment reads, distinguishing them from file-related properties.
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.
LGTM