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(cli): Store metrics_enabled key in profile config #536

Merged
merged 2 commits into from
Nov 17, 2024

Conversation

Souvik9205
Copy link
Contributor

@Souvik9205 Souvik9205 commented Nov 15, 2024

User description

Description

  • index.types.d.ts: Added metrics_enabled boolean to the ProfileConfig.
  • create.profile.ts: Introduced -m, --enable-metrics for enabling/disabling metrics collection.
  • update.profile.ts: Added the flag to update the metrics collection status.
  • list.profile.ts: Displayed Metrics Enabled column in the profile table
  • base.command.ts: Added metricsEnabled to store and manage the metrics setting.

Fixes #528

Developer's checklist

  • [✅] My PR follows the style guidelines of this project

  • [❌] I have performed a self-check on my work


PR Type

Enhancement


Description

  • Added a metrics_enabled key to the ProfileConfig interface to support metrics collection.
  • Introduced --enable-metrics flag in create.profile.ts for enabling/disabling metrics during profile creation.
  • Updated update.profile.ts to allow modification of the metrics setting for existing profiles.
  • Enhanced list.profile.ts to display the metrics enabled status in the profile list.
  • Modified base.command.ts to store and manage the metrics setting within the command base class.

Changes walkthrough 📝

Relevant files
Enhancement
base.command.ts
Add metricsEnabled property to BaseCommand class                 

apps/cli/src/commands/base.command.ts

  • Added metricsEnabled property to the BaseCommand class.
  • Updated logic to initialize metricsEnabled from profile configuration.

  • +3/-0     
    create.profile.ts
    Add enable-metrics flag to profile creation                           

    apps/cli/src/commands/profile/create.profile.ts

  • Introduced --enable-metrics flag for profile creation.
  • Updated input parsing to include metrics option.
  • Modified profile configuration to store metrics setting.
  • +30/-7   
    list.profile.ts
    Display metrics enabled status in profile list                     

    apps/cli/src/commands/profile/list.profile.ts

    • Added Metrics Enabled column to profile listing.
    +3/-2     
    update.profile.ts
    Add enable-metrics flag to profile update                               

    apps/cli/src/commands/profile/update.profile.ts

  • Added --enable-metrics flag for updating profiles.
  • Updated profile data to handle metrics setting.
  • +14/-3   
    index.types.d.ts
    Update ProfileConfig interface with metrics_enabled           

    apps/cli/src/types/index.types.d.ts

    • Added metrics_enabled boolean to ProfileConfig interface.
    +1/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    528 - Fully compliant

    Compliant requirements:

    • Added metrics_enabled to ProfileConfig interface
    • Added -m flag with prompt to create.profile.ts
    • Added metrics option to update.profile.ts
    • Added metrics column to list.profile.ts
    • Added metricsEnabled storage in base.command.ts
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Logic Error
    The condition for checking enableMetrics is incorrect. '!enableMetrics === undefined' is a confusing double negative that may not work as intended.

    Type Mismatch
    The setProfileConfigData function is called with setDefault parameter but the function signature was missing it in the old code. This could cause type errors.

    Incomplete Validation
    The enableMetrics parameter is used without validation. Should validate boolean type before using.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Fix incorrect logical condition that prevents the metrics confirmation prompt from being shown

    Fix the logical error in the metrics check condition. The current condition
    !enableMetrics === undefined is always false. It should be enableMetrics ===
    undefined.

    apps/cli/src/commands/profile/create.profile.ts [98-102]

    -if (!enableMetrics === undefined) {
    +if (enableMetrics === undefined) {
       enableMetrics = await confirm({
         message: 'Should keyshade collect anonymous metrics for development?'
       })
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The current condition !enableMetrics === undefined is a critical logical error that would prevent the metrics prompt from ever showing. This bug would effectively break the new metrics feature functionality.

    9
    Best practice
    Make new configuration field optional to maintain backward compatibility

    Make the metrics_enabled property optional in the ProfileConfig interface since it's
    a new field being added and may not exist in existing profiles.

    apps/cli/src/types/index.types.d.ts [10-14]

     [name: string]: {
       apiKey: string
       baseUrl: string
    -  metrics_enabled: boolean
    +  metrics_enabled?: boolean
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Making the metrics_enabled field optional is crucial for backward compatibility with existing profiles that don't have this field, preventing potential runtime errors when reading older configurations.

    8
    Possible issue
    Add input validation to ensure the metrics flag receives a valid boolean value

    Add validation for the enableMetrics boolean input parameter to ensure it's a valid
    boolean value before updating the profile.

    apps/cli/src/commands/profile/update.profile.ts [91-93]

     if (enableMetrics !== undefined) {
    -  this.profiles[profile].metrics_enabled = enableMetrics
    +  const metricsValue = String(enableMetrics).toLowerCase();
    +  if (metricsValue !== 'true' && metricsValue !== 'false') {
    +    throw new Error('Enable metrics must be a boolean value (true/false)');
    +  }
    +  this.profiles[profile].metrics_enabled = metricsValue === 'true';
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding validation for the metrics flag input would prevent potential runtime errors and improve user experience by providing clear error messages for invalid inputs.

    7

    💡 Need additional feedback ? start a PR chat

    @Souvik9205 Souvik9205 changed the title feat(cli):Store key in profile config feat(cli): Store key in profile config Nov 15, 2024
    @rajdip-b rajdip-b changed the title feat(cli): Store key in profile config feat(cli): Store metrics_enabled key in profile config Nov 17, 2024
    @rajdip-b rajdip-b merged commit 9283b22 into keyshade-xyz:develop Nov 17, 2024
    6 of 8 checks passed
    rajdip-b pushed a commit that referenced this pull request Dec 3, 2024
    ## [2.8.0](v2.7.0...v2.8.0) (2024-12-03)
    
    ### 🚀 Features
    
    * **api:** Add workspace removal notification email template ([#476](#476)) ([40b754f](40b754f))
    * **cli:** Store `metrics_enabled` key in profile config ([#536](#536)) ([9283b22](9283b22))
    * **package, api, cli:** Add api-key schemas and types; Fix schema inconsistencies; Minor fix for CLI build errors  ([#557](#557)) ([126d024](126d024))
    * **platform:** Added screen for CREATE NEW PROJECT ([#540](#540)) ([b644633](b644633))
    * **platform:** Updated the empty state of dashboard ([#522](#522)) ([28739d9](28739d9))
    * **schema, api-client:** Migrate auth types to @keyshade/schema ([#532](#532)) ([d880098](d880098))
    * **schema, api-client:** Migrate event schemas and types to @keyshade/schema ([#546](#546)) ([a3267de](a3267de))
    * **schema, api-client:** Migrate integration schemas and types to @keyshade/schema ([#547](#547)) ([08868c3](08868c3))
    * **schema, api-client:** Migrate project schemas and environment schemas along with their types to @keyshade/schema ([#538](#538)) ([c468af0](c468af0))
    * **schema, api-client:** Migrate [secure] types and schemas to @keyshade/schema ([#539](#539)) ([bc3100b](bc3100b))
    * **schema, api-client:** Migrate user types and schemas to @keyshade/schema ([#535](#535)) ([c24695e](c24695e))
    * **schema, api-client:** Migrate variable schemas and types to @keyshade/schema ([#545](#545)) ([0ee8f9a](0ee8f9a))
    * **schema, api-client:** Migrate workspace-membership schemas and types to @keyshade/schema ([#569](#569)) ([4398969](4398969))
    * **schema, api-client:** Migrate workspace-role schemas and types to @keyshade/schema ([#568](#568)) ([9efbf2d](9efbf2d))
    * **schema:** Add User type inference from UserSchema ([#574](#574)) ([84c1db5](84c1db5))
    
    ### 🐛 Bug Fixes
    
    * **api:** Incorrect oauth redirect url ([58d96e5](58d96e5))
    * **platform:** Resolve loading SVG blocking input field interaction ([#571](#571)) ([30f4f65](30f4f65))
    
    ### 📚 Documentation
    
    * Add pictures to Bruno setup ([#541](#541)) ([210c0fd](210c0fd))
    * Migrate to Bruno ([#525](#525)) ([1793d92](1793d92))
    
    ### 🔧 Miscellaneous Chores
    
    * **ci:** Add script to validate schema package ([59e4280](59e4280))
    * Fixed codecov client version ([a998ae4](a998ae4))
    * **package:** Fixed tests and did housekeeping ([#544](#544)) ([40008e3](40008e3))
    * Update test coverage settings ([5b27e32](5b27e32))
    * Update Turbo to 2.3.1 ([#564](#564)) ([3a63823](3a63823))
    * **web:** Update dockerfile ([10d9cc5](10d9cc5))
    
    ### 🔨 Code Refactoring
    
    * **api-client, schema:** Add workspace's schemas and types in @keyshade/schema ([#520](#520)) ([7c8ee5d](7c8ee5d))
    @Souvik9205 Souvik9205 deleted the matrics_enable branch December 24, 2024 13:16
    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.

    CLI: Store metrics_enabled key in profile config
    2 participants