Feature: Disable Secret Deletion Sync Option #3246
Conversation
WalkthroughThe changes introduce a new configuration property, Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
backend/src/services/secret-sync/aws-parameter-store/aws-parameter-store-sync-fns.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "/backend".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in "backend/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. backend/src/services/secret-sync/aws-secrets-manager/aws-secrets-manager-sync-fns.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "/backend".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in "backend/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. backend/src/services/secret-sync/azure-app-configuration/azure-app-configuration-sync-fns.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "/backend".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in "backend/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team.
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/src/hooks/api/secretSyncs/types/root-sync.ts (1)
6-6: LGTM - Type declaration looks goodThe
disableSecretDeletionproperty is properly defined as an optional boolean, maintaining backward compatibility with existing code.Consider adding JSDoc comments to provide more context about this property:
+ /** + * When enabled, prevents Infisical from removing secrets from the sync destination + * that do not exist in Infisical. Useful when managing some secrets manually outside of Infisical. + */ disableSecretDeletion?: boolean;backend/src/services/secret-sync/humanitec/humanitec-sync-fns.ts (1)
199-200:❓ Verification inconclusive
LGTM - Implementation looks correct
The early return prevents secret deletion when the
disableSecretDeletionoption is enabled, which aligns with the feature's purpose.Consider adding a comment to clarify the purpose of this check for future maintainers:
+ // Skip deletion of secrets not present in Infisical when disableSecretDeletion is enabled if (secretSync.syncOptions.disableSecretDeletion) return;Also, verify that the feature is properly tested to ensure it works as expected in various scenarios.
🏁 Script executed:
#!/bin/bash # Check if there are tests covering the new disableSecretDeletion functionality rg -A 3 -B 3 "disableSecretDeletion.*true" --type tsLength of output: 54
Action: Add clarifying comment and verify test coverage manually
The early return implementation is correct. To improve maintainability, please add a comment above the return to clarify that secret deletion is skipped when
disableSecretDeletionis enabled. For example:+ // Skip deletion of secrets not present in Infisical when disableSecretDeletion is enabled if (secretSync.syncOptions.disableSecretDeletion) return;Additionally, our automated search did not reveal any tests covering this functionality. Can you please verify manually that tests exist to validate that secrets are not deleted when
disableSecretDeletionis active?backend/src/services/secret-sync/aws-parameter-store/aws-parameter-store-sync-fns.ts (1)
385-385: Effective implementation of deletion prevention.The early return approach efficiently prevents secret deletion when the option is enabled. This implementation correctly bypasses the entire deletion process, including the collecting of parameters to delete and the subsequent batch deletion operation.
However, note that this implementation differs slightly from the GCP implementation (return vs. continue). While both achieve the same goal, ensuring consistency in implementation patterns across similar functions can aid maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
backend/src/lib/api-docs/constants.ts(1 hunks)backend/src/services/secret-sync/aws-parameter-store/aws-parameter-store-sync-fns.ts(1 hunks)backend/src/services/secret-sync/aws-secrets-manager/aws-secrets-manager-sync-fns.ts(1 hunks)backend/src/services/secret-sync/azure-app-configuration/azure-app-configuration-sync-fns.ts(1 hunks)backend/src/services/secret-sync/azure-key-vault/azure-key-vault-sync-fns.ts(1 hunks)backend/src/services/secret-sync/databricks/databricks-sync-fns.ts(1 hunks)backend/src/services/secret-sync/gcp/gcp-sync-fns.ts(1 hunks)backend/src/services/secret-sync/github/github-sync-fns.ts(1 hunks)backend/src/services/secret-sync/humanitec/humanitec-sync-fns.ts(1 hunks)backend/src/services/secret-sync/secret-sync-schemas.ts(1 hunks)docs/integrations/secret-syncs/aws-parameter-store.mdx(1 hunks)docs/integrations/secret-syncs/aws-secrets-manager.mdx(1 hunks)docs/integrations/secret-syncs/azure-app-configuration.mdx(2 hunks)docs/integrations/secret-syncs/azure-key-vault.mdx(2 hunks)docs/integrations/secret-syncs/databricks.mdx(1 hunks)docs/integrations/secret-syncs/gcp-secret-manager.mdx(1 hunks)docs/integrations/secret-syncs/github.mdx(1 hunks)docs/integrations/secret-syncs/humanitec.mdx(1 hunks)frontend/src/components/secret-syncs/forms/SecretSyncOptionsFields/SecretSyncOptionsFields.tsx(2 hunks)frontend/src/components/secret-syncs/forms/SecretSyncReviewFields/SecretSyncReviewFields.tsx(2 hunks)frontend/src/components/secret-syncs/forms/schemas/base-secret-sync-schema.ts(1 hunks)frontend/src/hooks/api/secretSyncs/types/root-sync.ts(1 hunks)frontend/src/pages/secret-manager/SecretSyncDetailsByIDPage/components/SecretSyncOptionsSection/SecretSyncOptionsSection.tsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Check Frontend Type and Lint check
- GitHub Check: Check TS and Lint
- GitHub Check: Run integration test
🔇 Additional comments (30)
docs/integrations/secret-syncs/gcp-secret-manager.mdx (1)
46-46: LGTM - Good addition to sync optionsThe new "Disable Secret Deletion" option is clearly documented with a good explanation of its purpose and when to use it.
docs/integrations/secret-syncs/humanitec.mdx (1)
59-59: LGTM - Documentation is consistentThe "Disable Secret Deletion" option is consistently documented across integrations with clear explanations.
backend/src/services/secret-sync/databricks/databricks-sync-fns.ts (1)
115-116: Good implementation of the disable-deletion feature.The added conditional check will prevent secret deletion when the
disableSecretDeletionoption is set, allowing users to maintain manually managed secrets outside of Infisical while still syncing secrets from Infisical.docs/integrations/secret-syncs/github.mdx (1)
66-67: Documentation is clear and accurately describes the new feature.The added description for the "Disable Secret Deletion" option clearly explains its purpose and when users should enable it. This is helpful for users who need to maintain some secrets manually outside of Infisical.
backend/src/services/secret-sync/aws-secrets-manager/aws-secrets-manager-sync-fns.ts (1)
399-400: The disable-deletion feature is correctly implemented.The added conditional check prevents secrets from being deleted when the
disableSecretDeletionflag is set to true. The implementation is consistent with other sync services and properly placed before the deletion loop.backend/src/lib/api-docs/constants.ts (1)
1729-1730: API documentation correctly includes the new sync option.The
disableSecretDeletionoption has been properly documented with a clear description of its purpose. This ensures API users will have accurate information about this feature.backend/src/services/secret-sync/azure-app-configuration/azure-app-configuration-sync-fns.ts (1)
139-140: Correctly implemented skip condition for secret deletionThe addition of this conditional check properly implements the feature to disable secret deletion during synchronization. When
disableSecretDeletionis enabled in sync options, the function will return early, preventing execution of the code that would delete secrets from Azure App Configuration.backend/src/services/secret-sync/azure-key-vault/azure-key-vault-sync-fns.ts (1)
192-193: Correctly implemented skip condition for secret deletionThis conditional check properly implements the feature to disable secret deletion during synchronization with Azure Key Vault. The early return prevents execution of the deletion logic when the
disableSecretDeletionoption is enabled. The implementation is consistent with other integration services.frontend/src/components/secret-syncs/forms/SecretSyncReviewFields/SecretSyncReviewFields.tsx (2)
39-39: Correctly added new option to watched propertiesThe
disableSecretDeletionproperty is properly added to the destructured properties from the form context, making it available for use in the component.
115-119: Well implemented UI feedback for the new optionThe conditional rendering of the "Disable Secret Deletion" label with a success badge provides clear visual feedback to users when they've enabled this feature. The implementation follows the established pattern used for other options in this component.
frontend/src/components/secret-syncs/forms/schemas/base-secret-sync-schema.ts (1)
10-11: Properly defined schema for the new optionThe
disableSecretDeletionproperty is correctly defined in the schema as an optional boolean that defaults tofalse. This ensures the feature is opt-in, which is appropriate for functionality that modifies default behavior.docs/integrations/secret-syncs/aws-secrets-manager.mdx (2)
49-49: Helpful tag precedence clarification.This note adds valuable clarification about how tag conflicts are resolved, making it clear to users that manually configured tags take precedence over automatically generated tags from metadata.
51-51: Good addition of the Secret Deletion control feature.This new option enhances user control by allowing them to maintain manually managed secrets alongside those synchronized by Infisical. This addition directly addresses the PR objective of preventing removal of manually managed secrets outside the platform.
backend/src/services/secret-sync/gcp/gcp-sync-fns.ts (1)
150-152: Well-implemented secret deletion prevention.The conditional check properly implements the "Disable Secret Deletion" feature for GCP secrets. By using a
continuestatement, it correctly skips over the deletion logic without interfering with other operations.docs/integrations/secret-syncs/databricks.mdx (1)
50-50: Consistent documentation of new feature.The addition of the "Disable Secret Deletion" option maintains consistency with other integration documentation, providing the same capability across different platforms.
backend/src/services/secret-sync/github/github-sync-fns.ts (1)
221-228: Well-implemented feature for optional secret deletion bypassingThe added conditional check for
disableSecretDeletioncreates a clear guard clause that properly implements the feature described in the PR objectives. When enabled, this prevents Infisical from removing secrets that might be managed manually outside the platform, which is an excellent addition for environments with mixed secret management workflows.The code structure is clean and follows the existing pattern with a straightforward early return when deletion should be skipped.
docs/integrations/secret-syncs/azure-key-vault.mdx (2)
9-9: Improved connection path referenceThe link now properly directs to the specific Azure Key Vault connection documentation rather than the general Azure connection page. The grammar correction from "a Azure" to "an Azure" is also appropriate.
55-55: Clear documentation for the new featureThe added description for the "Disable Secret Deletion" option clearly explains both the functionality and the use case for this feature, matching the implementation in the code.
backend/src/services/secret-sync/secret-sync-schemas.ts (1)
26-27: Schema properly extended to support the new optionThe schema definition for
disableSecretDeletionis correctly implemented as an optional boolean field with appropriate description reference. This ensures proper validation in API requests and consistent documentation.docs/integrations/secret-syncs/aws-parameter-store.mdx (2)
46-46: Improved formatting for note elementThe note about tag precedence has been reformatted for better consistency with the document structure.
48-48: Consistent documentation of the new featureThe description for the "Disable Secret Deletion" option matches the one in other integration pages, ensuring consistent documentation of this feature across the platform.
frontend/src/pages/secret-manager/SecretSyncDetailsByIDPage/components/SecretSyncOptionsSection/SecretSyncOptionsSection.tsx (4)
7-7: Proper import update for Badge component.The Badge component has been correctly imported to support displaying the "Enabled" status for the new disableSecretDeletion option.
27-28: New configuration property added correctly.The disableSecretDeletion property has been properly added to the destructured syncOptions object, aligning with the PR objective of adding a feature to disable the deletion of secrets during synchronization.
62-77: Component structure improvement.The ProjectPermissionCan component rendering has been refactored to simplify the control flow, making it more consistent by always rendering the edit button based on user permissions rather than conditionally checking for AdditionalSyncOptionsComponent.
87-91: Well-implemented UI for the new feature.The conditional rendering of the disableSecretDeletion status with a success badge is clean and consistent with the UI design patterns in the application.
docs/integrations/secret-syncs/azure-app-configuration.mdx (2)
9-9: Clarified connection type for better user guidance.The prerequisite has been updated to specify "Azure App Configuration Connection" instead of the more general "Azure Connection," making it clearer which specific connection type is required for setup.
53-53: Clear documentation for the new feature.The documentation for the "Disable Secret Deletion" feature clearly explains its purpose and when to use it. The explanation that "Infisical will not remove secrets from the sync destination" and that this is useful "if you intend to manage some secrets manually outside of Infisical" provides users with the necessary context to make informed decisions.
frontend/src/components/secret-syncs/forms/SecretSyncOptionsFields/SecretSyncOptionsFields.tsx (3)
3-3: Updated imports to support new UI elements.The faQuestionCircle icon has been properly imported to provide helpful tooltips for the new feature.
6-6: Added necessary UI components for the feature.Switch and Tooltip components have been correctly imported to implement the UI for enabling/disabling secret deletion.
119-156: Well-implemented form control for the new feature.The implementation of the disableSecretDeletion option is thorough and user-friendly:
- The Switch component is styled consistently with other form elements
- The tooltip provides clear explanation of the feature's purpose
- Error handling is properly implemented
- The copy explains both what the feature does and when it should be used
This implementation aligns perfectly with the PR's objective of allowing users to disable the deletion of secrets during synchronization.
Description 📣
This PR adds the ability to disable secret deletion for secret syncs on sync. This option prevents manually managed secrets outside of Infisical from being removed. Doc additions and fixes included.
Type ✨
Tests 🛠️
# Here's some code block to paste some code snippetsSummary by CodeRabbit
New Features
Documentation