feat(secrets): add secrets to system properties #5
Conversation
WalkthroughThis pull request updates the GitHub Actions workflow to use a newer version of the artifact upload action, modifies the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SecretsClient
participant System
Client->>SecretsClient: call ListSecrets(..., setSecretsOnSystemProperties)
alt setSecretsOnSystemProperties is true
SecretsClient->>System: System.setProperty(secretKey, secretValue)
end
SecretsClient-->>Client: return List of secrets
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
src/test/java/com/infisical/sdk/InfisicalSdkTest.java:34
- [nitpick] Consider adding a dedicated test case that calls ListSecrets with setSecretsOnSystemProperties set to true and verifies that the corresponding system properties are set as expected.
var secrets = sdk.Secrets().ListSecrets(envVars.getProjectId(), "dev", "/", false, false, false, false);
src/main/java/com/infisical/sdk/resources/SecretsClient.java:33
- [nitpick] It would be beneficial to include inline comments explaining the side effect of setting system properties for clarity and to ensure related functionality is covered by tests.
if (setSecretsOnSystemProperties) {
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/main/java/com/infisical/sdk/resources/SecretsClient.java (1)
69-70:⚠️ Potential issueFix potential bug in UpdateSecret method.
There appears to be a duplicated condition check with inconsistent property setting. Both lines check if
newSecretNameis not null, but one setsnewSecretNameand the other setssecretValue. The second line probably should check ifnewSecretValueis not null instead.- if (newSecretName != null) inputBuilder.newSecretName(newSecretName); - if (newSecretName != null) inputBuilder.secretValue(newSecretValue); + if (newSecretName != null) inputBuilder.newSecretName(newSecretName); + if (newSecretValue != null) inputBuilder.secretValue(newSecretValue);
🧹 Nitpick comments (1)
src/main/java/com/infisical/sdk/resources/SecretsClient.java (1)
91-91: Remove redundant check and assignment in CreateSecret method.Line 91 unnecessarily sets the secret value that was already set on line 88 during the builder construction. If the intent is to handle empty strings differently, the check should happen before the initial builder setup.
- createSecretInput.setSecretValue(!secretValue.isEmpty() ? secretValue : "");Or if there is a need to ensure empty strings are handled properly, move the check before the builder:
var createSecretInput = CreateSecretInput.builder() .secretPath(secretPath) .projectId(projectId) .environmentSlug(environmentSlug) - .secretValue(secretValue) + .secretValue(!secretValue.isEmpty() ? secretValue : "") .build(); - createSecretInput.setSecretValue(!secretValue.isEmpty() ? secretValue : "");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.DS_Storeis excluded by!**/.DS_Store
📒 Files selected for processing (5)
.github/workflows/test.yml(1 hunks).gitignore(1 hunks)README.md(3 hunks)src/main/java/com/infisical/sdk/resources/SecretsClient.java(2 hunks)src/test/java/com/infisical/sdk/InfisicalSdkTest.java(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/test/java/com/infisical/sdk/InfisicalSdkTest.java (4)
src/main/java/com/infisical/sdk/config/SdkConfig.java (1)
SdkConfig(3-30)src/test/java/com/infisical/sdk/util/EnvironmentVariables.java (1)
EnvironmentVariables(6-44)src/main/java/com/infisical/sdk/util/InfisicalException.java (1)
InfisicalException(8-29)src/test/java/com/infisical/sdk/util/RandomUtil.java (1)
RandomUtil(6-21)
🔇 Additional comments (6)
.gitignore (1)
3-4: Good housekeeping additions.Adding
.idea(JetBrains IDE files) and.DS_Store(macOS system files) to the gitignore is a good practice that helps keep the repository clean..github/workflows/test.yml (1)
41-41: Good maintenance update.Updating to the latest version (v4) of the upload-artifact GitHub Action follows good dependency management practices.
src/test/java/com/infisical/sdk/InfisicalSdkTest.java (1)
4-12: LGTM: Import reorganization.The imports have been reorganized appropriately.
README.md (3)
117-119: Method signature updated correctly.The
ListSecretsmethod signature has been updated to include the new parameter.
132-133: Example usage is clear and well-documented.The example usage now includes the new parameter with a descriptive comment explaining its purpose.
143-143: Parameter documentation is thorough.The added parameter documentation clearly explains the purpose and behavior of the new
setSecretsOnSystemPropertiesparameter.
2 participants
This PR adds support for setting secrets retrieved by
ListSecrets, to the system properties, making them accessible through System.getProperty(secretKey)`.Ideally we'd want it to be accessible on os.Getenv(), but in Java the environment is immutable, so this isn't possible for our SDK. If we had built more framework-targeted SDK's (like a specific springboot SDK), we could've achieved this. But our SDK is general-purpose, so we can't narrow it down to certain framework implementations.
Summary by CodeRabbit
New Features
Documentation
Chores