Closed anudeeps352 closed 3 weeks ago
Here are some key observations to aid the review process:
**π« Ticket compliance analysis β ** **[301](https://github.com/keyshade-xyz/keyshade/issues/301) - Fully compliant** Fully compliant requirements: - List all variables under a project - Fetch all revisions of a variable - Create a variable - Update a variable - Rollback a variable - Delete a variable - Create an implementation of `command.interface.ts` and name it `VariableCommand` - Stash all the functions in `src/commands/variable` - Use the `variableController` from `ControllerInstance` to make the API calls |
β±οΈ Estimated effort to review: 4 π΅π΅π΅π΅βͺ |
π§ͺ No relevant tests |
π No security concerns identified |
β‘ Recommended focus areas for review Error Handling The error handling for the 'entries' option could be improved. Currently, it throws an error if entries are not provided, which might not be the best user experience. Type Safety The use of 'any' type when iterating over variables could lead to potential type-related issues. Consider using a more specific type. Code Duplication The logging of revision details is verbose and could be refactored into a separate method to improve readability and maintainability. |
Explore these optional code suggestions:
Category | Suggestion | Score |
Possible issue |
Implement input validation and type checking for the update options to enhance data integrity and prevent runtime errors___ **Theoptions object is passed directly to the updateVariable method without any validation or type checking. Consider adding input validation and type checking before passing the options to ensure data integrity and prevent potential runtime errors.** [apps/cli/src/commands/variable/update.variable.ts [51-58]](https://github.com/keyshade-xyz/keyshade/pull/514/files#diff-e081dceac29d37193bb60782e1cbfffc930128f4f0d752af0946baac3256e776R51-R58) ```diff +interface UpdateVariableOptions { + name?: string; + note?: string; + entries?: string[]; +} + +const validatedOptions: UpdateVariableOptions = {}; +if (typeof options.name === 'string') validatedOptions.name = options.name; +if (typeof options.note === 'string') validatedOptions.note = options.note; +if (Array.isArray(options.entries)) validatedOptions.entries = options.entries; + const { data, error, success } = await ControllerInstance.getInstance().variableController.updateVariable( { variableSlug, - ...options + ...validatedOptions }, this.headers ) ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 9Why: Adding input validation and type checking for the update options is crucial for ensuring data integrity and preventing runtime errors, making this a high-impact improvement. | 9 |
Enhancement |
β Improve the robustness and error handling of the entries parsing logic___ **Consider using a more robust method for parsing the entries input. The currentimplementation assumes a specific format and may fail for more complex inputs. Consider using a library like 'yargs' for parsing command-line arguments or implement a more flexible parsing logic.** [apps/cli/src/commands/variable/create.variable.ts [101-111]](https://github.com/keyshade-xyz/keyshade/pull/514/files#diff-bc3afb04230c3c629f17dbd1fc5f734e65134bf61ad18ea26d65998b948b9ff7R101-R111) ```diff const parsedEntries = entries.map((entry) => { - const entryObj: { value: string; environmentSlug: string } = { - value: '', - environmentSlug: '' + const [environmentSlug, value] = entry.split('=').map(s => s.trim()) + if (!environmentSlug || !value) { + throw new Error(`Invalid entry format: ${entry}. Expected format: "environmentSlug=value"`) } - entry.split(' ').forEach((pair) => { - const [key, value] = pair.split('=') - entryObj[key] = value - }) - return entryObj + return { environmentSlug, value } }) ``` `[Suggestion has been applied]` Suggestion importance[1-10]: 8Why: The suggestion enhances the robustness of the entries parsing logic by introducing error handling for invalid formats, which can prevent potential runtime errors and improve the user experience. | 8 |
Enhance type safety and readability by defining a Revision interface and using template literals___ **Similar to the previous suggestion, consider defining an interface for the revisionstructure to improve type safety and code readability. Also, use template literals for better string formatting.** [apps/cli/src/commands/variable/revisions.variable.ts [47-55]](https://github.com/keyshade-xyz/keyshade/pull/514/files#diff-0418e51a07f5eb276c22c750a6ff91907cc2578e7225282cc8d7df32b8a64a18R47-R55) ```diff -data.items.forEach((revision: any) => { - Logger.info(`Id ${revision.id}`) - Logger.info(`value ${revision.value}`) - Logger.info(`version ${revision.version}`) - Logger.info(`variableID ${revision.variableId}`) - Logger.info(`Created On ${revision.createdOn}`) - Logger.info(`Created By Id ${revision.createdById}`) - Logger.info(`environmentId ${revision.environmentId}`) +interface Revision { + id: string; + value: string; + version: string; + variableId: string; + createdOn: string; + createdById: string; + environmentId: string; +} + +data.items.forEach((revision: Revision) => { + Logger.info(`Id: ${revision.id}`) + Logger.info(`Value: ${revision.value}`) + Logger.info(`Version: ${revision.version}`) + Logger.info(`Variable ID: ${revision.variableId}`) + Logger.info(`Created On: ${revision.createdOn}`) + Logger.info(`Created By ID: ${revision.createdById}`) + Logger.info(`Environment ID: ${revision.environmentId}`) }) ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: The suggestion improves type safety and readability by defining a Revision interface and using template literals, which can make the code more maintainable and less error-prone. | 7 | |
Best practice |
Improve type safety by explicitly defining the structure of the variable object___ **Thevariable type in the forEach loop is implicitly any . Consider defining an interface for the variable structure and use it to type the variable parameter, improving type safety and code readability.** [apps/cli/src/commands/variable/list.variable.ts [40-42]](https://github.com/keyshade-xyz/keyshade/pull/514/files#diff-10b95c97a99af739c47041522cb44cdc5bb784e26faa42731ea78b27e894c9c8R40-R42) ```diff -data.forEach((variable: any) => { +interface Variable { + name: string; + value: string; +} + +data.forEach((variable: Variable) => { Logger.info(`- ${variable.name} (${variable.value})`) }) ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: Defining an interface for the variable structure enhances type safety and code readability, which is beneficial for maintaining and understanding the code. | 7 |
π‘ Need additional feedback ? start a PR chat
:tada: This PR is included in version 2.7.0 :tada:
The release is available on GitHub release
Your semantic-release bot :package::rocket:
User description
Description
Add functionality to operate on Variables
Fixes #301
Developer's checklist
If changes are made in the code:
Documentation Update
PR Type
Enhancement
Description
VariableCommand
class to manage variables through the CLI, including subcommands for creating, deleting, listing, fetching revisions, rolling back, and updating variables.VariableCommand
into the CLI application, making it available for use.Changes walkthrough π
variable.command.ts
Add `VariableCommand` class for CLI variable operations
apps/cli/src/commands/variable.command.ts
VariableCommand
class extendingBaseCommand
.create.variable.ts
Implement `CreateVariable` command for CLI
apps/cli/src/commands/variable/create.variable.ts
CreateVariable
class for creating variables.delete.variable.ts
Implement `DeleteVariable` command for CLI
apps/cli/src/commands/variable/delete.variable.ts
DeleteVariable
class for deleting variables.list.variable.ts
Implement `ListVariable` command for CLI
apps/cli/src/commands/variable/list.variable.ts
ListVariable
class for listing variables.revisions.variable.ts
Implement `FetchVariableRevisions` command for CLI
apps/cli/src/commands/variable/revisions.variable.ts
FetchVariableRevisions
class for fetching variablerevisions.
rollback.variable.ts
Implement `RollbackVariable` command for CLI
apps/cli/src/commands/variable/rollback.variable.ts
RollbackVariable
class for rolling back variables.update.variable.ts
Implement `UpdateVariable` command for CLI
apps/cli/src/commands/variable/update.variable.ts
UpdateVariable
class for updating variables.index.ts
Integrate `VariableCommand` into CLI application
apps/cli/src/index.ts
VariableCommand
into the CLI application.VariableCommand
to the list of available commands.