Skip to content
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

feat(api): Add requireRestart parameter #286

Merged
merged 5 commits into from
Jun 26, 2024

Conversation

yogesh1801
Copy link
Contributor

@yogesh1801 yogesh1801 commented Jun 25, 2024

User description

Description

Added requireRestart param

Fixes #283

Developer's checklist

  • My PR follows the style guidelines of this project
  • I have performed a self-check on my work

If changes are made in the code:

  • I have followed the coding guidelines
  • My changes in code generate no new warnings
  • My changes are breaking another fix/feature of the project
  • I have added test cases to show that my feature works
  • I have added relevant screenshots in my PR
  • There are no UI/UX issues

Documentation Update

  • This PR requires an update to the documentation at docs.keyshade.xyz
  • I have made the necessary updates to the documentation, or no documentation changes are required.

PR Type

Enhancement, Documentation


Description

  • Added requireRestart boolean property to CreateSecret and CreateVariable classes.
  • Integrated requireRestart in secret and variable service operations, including creation, update, and rollback.
  • Updated Prisma schema and migration scripts to include requireRestart field in Secret and Variable models.
  • Added missing newline at the end of workspace-storage.ts file.

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

    Signed-off-by: yogesh1801 <yogeshsingla481@gmail.com>
    Copy link
    Contributor

    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.

    Copy link
    Contributor

    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]

    -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]

    -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]

     @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

    @yogesh1801 yogesh1801 changed the title feature: added requireRestart param feat: added requireRestart param Jun 25, 2024
    @rajdip-b
    Copy link
    Member

    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
    Copy link
    Contributor Author

    yogesh1801 commented Jun 25, 2024

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

    Signed-off-by: yogesh1801 <yogeshsingla481@gmail.com>
    Signed-off-by: yogesh1801 <yogeshsingla481@gmail.com>
    apps/api/src/prisma/schema.prisma Outdated Show resolved Hide resolved
    Copy link
    Member

    @rajdip-b rajdip-b left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Could you also add these few test cases?

    • Creating a secret with requireRestart set to true
    • Creating a variable with requireRestart set to true
    • Updating a secret with requireRestart set to true
    • Updating a variable with requireRestart set to true

    Copy link
    Member

    @rajdip-b rajdip-b left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Update the interface over here

    @rajdip-b rajdip-b changed the title feat: added requireRestart param feat(api): Add requireRestart parameter Jun 26, 2024
    Signed-off-by: yogesh1801 <yogeshsingla481@gmail.com>
    Signed-off-by: yogesh1801 <yogeshsingla481@gmail.com>
    @rajdip-b rajdip-b merged commit 39525ce into keyshade-xyz:develop Jun 26, 2024
    4 checks passed
    @yogesh1801 yogesh1801 deleted the pr/added-requireRestart branch June 26, 2024 10:29
    rajdip-b pushed a commit that referenced this pull request Jun 27, 2024
    Signed-off-by: yogesh1801 <yogeshsingla481@gmail.com>
    rajdip-b pushed a commit that referenced this pull request Jun 27, 2024
    ## [2.1.0](v2.0.0...v2.1.0) (2024-06-27)
    
    ### 🚀 Features
    
    * **api:** Add `requireRestart` parameter ([#286](#286)) ([fb447a1](fb447a1))
    * **cli:** Added CLI ([#289](#289)) ([1143d95](1143d95))
    * **workflows:** Tag user on attempt's reply body ([9d01698](9d01698))
    
    ### 🐛 Bug Fixes
    
    * **web:** Resolve encryption glitch in footer text  ([#267](#267)) ([2b5cb39](2b5cb39))
    
    ### 📚 Documentation
    
    * added running-the-web-app.md ([#269](#269)) ([755ea12](755ea12))
    rajdip-b pushed a commit that referenced this pull request Jun 27, 2024
    ## [2.1.0](v2.0.0...v2.1.0) (2024-06-27)
    
    ### 🚀 Features
    
    * **api:** Add `requireRestart` parameter ([#286](#286)) ([fb447a1](fb447a1))
    * **cli:** Added CLI ([#289](#289)) ([1143d95](1143d95))
    * **workflows:** Tag user on attempt's reply body ([9d01698](9d01698))
    
    ### 🐛 Bug Fixes
    
    * **web:** Resolve encryption glitch in footer text  ([#267](#267)) ([2b5cb39](2b5cb39))
    
    ### 📚 Documentation
    
    * added running-the-web-app.md ([#269](#269)) ([755ea12](755ea12))
    yogesh1801 pushed a commit to yogesh1801/keyshade that referenced this pull request Jun 29, 2024
    ## [2.1.0](keyshade-xyz/keyshade@v2.0.0...v2.1.0) (2024-06-27)
    
    ### 🚀 Features
    
    * **api:** Add `requireRestart` parameter ([keyshade-xyz#286](keyshade-xyz#286)) ([fb447a1](keyshade-xyz@fb447a1))
    * **cli:** Added CLI ([keyshade-xyz#289](keyshade-xyz#289)) ([1143d95](keyshade-xyz@1143d95))
    * **workflows:** Tag user on attempt's reply body ([9d01698](keyshade-xyz@9d01698))
    
    ### 🐛 Bug Fixes
    
    * **web:** Resolve encryption glitch in footer text  ([keyshade-xyz#267](keyshade-xyz#267)) ([2b5cb39](keyshade-xyz@2b5cb39))
    
    ### 📚 Documentation
    
    * added running-the-web-app.md ([keyshade-xyz#269](keyshade-xyz#269)) ([755ea12](keyshade-xyz@755ea12))
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    API: Add requiresRestart field to Secret and Variable
    2 participants