keyshade-xyz / keyshade

Realtime secret and configuration management tool, with the best in class security and seamless integration support
https://keyshade.xyz
Mozilla Public License 2.0
196 stars 96 forks source link

feat(api): Add `requireRestart` parameter #286

Closed yogesh1801 closed 3 months ago

yogesh1801 commented 3 months ago

User description

Description

Added requireRestart param

Fixes #283

Developer's checklist

If changes are made in the code:

Documentation Update


PR Type

Enhancement, Documentation


Description


Changes walkthrough ๐Ÿ“

Relevant files
Enhancement
create.secret.ts
Add `requireRestart` property to `CreateSecret` class       

apps/api/src/secret/dto/create.secret/create.secret.ts
  • Added requireRestart boolean property to CreateSecret class.
  • Marked requireRestart as optional.
  • +5/-0     
    secret.service.ts
    Integrate `requireRestart` in secret service operations   

    apps/api/src/secret/service/secret.service.ts
  • Included requireRestart in secret creation and update logic.
  • Added requireRestart to the payload for secret rollback.
  • +4/-0     
    create.variable.ts
    Add `requireRestart` property to `CreateVariable` class   

    apps/api/src/variable/dto/create.variable/create.variable.ts
  • Added requireRestart boolean property to CreateVariable class.
  • Marked requireRestart as optional.
  • +5/-0     
    variable.service.ts
    Integrate `requireRestart` in variable service operations

    apps/api/src/variable/service/variable.service.ts
  • Included requireRestart in variable creation and update logic.
  • Added requireRestart to the payload for variable rollback.
  • +4/-0     
    migration.sql
    Add `requireRestart` column to database schema                     

    apps/api/src/prisma/migrations/20240625190757_new/migration.sql
  • Added requireRestart column to Secret and Variable tables with default
    value false.
  • +5/-0     
    schema.prisma
    Update Prisma schema with `requireRestart` field                 

    apps/api/src/prisma/schema.prisma
  • Added requireRestart boolean field to Secret and Variable models with
    default value false.
  • +2/-0     
    Formatting
    workspace-storage.ts
    Add newline at end of workspace storage file                         

    apps/platform/src/lib/workspace-storage.ts - Added missing newline at the end of the file.
    +1/-1     

    ๐Ÿ’ก PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    codiumai-pr-agent-free[bot] commented 3 months ago

    PR Reviewer Guide ๐Ÿ”

    โฑ๏ธ Estimated effort to review [1-5] 3
    ๐Ÿงช Relevant tests No
    ๐Ÿ”’ Security concerns No
    โšก Key issues to review Possible Bug:
    The requireRestart field is optional in DTOs but not nullable in the Prisma schema for the Variable model. This inconsistency could lead to runtime errors when requireRestart is not provided.
    Data Integrity:
    The default behavior for requireRestart in the Secret model is nullable with a default of false, which is inconsistent with the Variable model where it is non-nullable. Consider aligning these behaviors to maintain consistency across similar features.
    codiumai-pr-agent-free[bot] commented 3 months ago

    PR Code Suggestions โœจ

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add a null check for requireRestart to prevent runtime errors ___ **Add a null check for dto.requireRestart before using it in the create method to prevent
    potential runtime errors if the field is not provided in the DTO.** [apps/api/src/secret/service/secret.service.ts [91]](https://github.com/keyshade-xyz/keyshade/pull/286/files#diff-dee38177617754972e3d3f727d7f1536566d7a784b7ffdda74aa97d6eec4cbc5R91-R91) ```diff -requireRestart: dto.requireRestart, +requireRestart: dto.requireRestart ?? false, ```
    Suggestion importance[1-10]: 10 Why: Adding a null check for `requireRestart` before using it in the `create` method is crucial to prevent potential runtime errors if the field is not provided. This is an important fix for ensuring the stability of the application.
    10
    Possible issue
    Make the requireRestart field optional in the Variable model ___ **Ensure consistency in the database schema by making the requireRestart field optional in
    the Variable model, similar to its definition in the Secret model. This change will help
    maintain uniformity and potentially avoid runtime errors due to null values.** [apps/api/src/prisma/schema.prisma [404]](https://github.com/keyshade-xyz/keyshade/pull/286/files#diff-24191263d57c55d02e0cdf394de15225b628d1da20bd826b1541b59b6b02adb4R404-R404) ```diff -requireRestart Boolean @default(false) +requireRestart Boolean? @default(false) ```
    Suggestion importance[1-10]: 9 Why: Ensuring consistency between the `Secret` and `Variable` models by making the `requireRestart` field optional helps maintain uniformity and prevents potential runtime errors due to null values. This is a significant improvement for consistency and reliability.
    9
    Enhancement
    Initialize the requireRestart field with a default value ___ **Consider initializing the requireRestart field with a default value to ensure consistent
    behavior across different parts of the application where this DTO might be used without
    explicitly setting this field.** [apps/api/src/secret/dto/create.secret/create.secret.ts [27]](https://github.com/keyshade-xyz/keyshade/pull/286/files#diff-0ba28109a277c57e22c8bdc661163b0d1f3b4ae8171a89d3fc72cf3e620529cbR27-R27) ```diff @IsOptional() -requireRestart?: boolean +requireRestart?: boolean = false ```
    Suggestion importance[1-10]: 8 Why: Initializing the `requireRestart` field with a default value ensures consistent behavior and avoids potential issues when the field is not explicitly set. This is a good enhancement for robustness.
    8
    rajdip-b commented 3 months ago

    Hey bro! Glad that you put up the PR. Are you attempting this for FOSS hack? If so, the PR will need to wait. If not, we can start reviewing it asap!

    yogesh1801 commented 3 months ago

    @rajdip-b this PR is my attempt to get familiar with the codebase, will attempt fosshack later, you can review this PR