init release manager page #427
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes add new relationship definitions using the drizzle-orm library for deployment and release entities. A new package, Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant RNV as releaseNewVersion()
participant GSR as getSystemResources()
participant RM as ReleaseManager
participant DR as DatabaseReleaseRepository
C->>RNV: Call releaseNewVersion(tx, versionId)
RNV->>RNV: Query deployment version & system details
RNV->>GSR: getSystemResources(tx, systemId)
GSR-->>RNV: Return list of system resources
RNV->>RM: Create ReleaseManager for each resource
RM->>DR: upsertRelease(versionId) to check/create release
alt setAsDesired true
RM->>DR: setDesiredRelease(desiredReleaseId)
end
DR-->>RM: Release upserted/updated
RM-->>RNV: Confirm release for resource
RNV-->>C: New version release completed
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
✨ 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (14)
packages/release-manager/src/types.ts (1)
1-6: Consider adding a more specific default type for VariableThe
Variable<T = any>type has a default ofany, which reduces type safety. Consider using a more specific default type if possible, such asunknownor a union of common value types.-export type Variable<T = any> = { +export type Variable<T = unknown> = { id: string; key: string; value: T; sensitive: boolean; };packages/release-manager/src/variables.ts (2)
23-29: Consider documenting provider priority in getVariable methodThe
getVariablemethod returns the first non-null variable found from providers, which means the order of providers matters for resolution. This behavior should be documented to make it clear for users.async getVariable(key: string): Promise<MaybeVariable> { + // Iterates through providers in order, returning the first non-null variable found + // This means provider order determines resolution priority for (const provider of this.variableProviders) { const variable = await provider.getVariable(key); if (variable) return variable; } return null; }
16-21: Consider filtering out null variables from getVariables resultThe
getVariablesmethod returns an array that might include null values, which could lead to unexpected behavior for consumers who expect only valid variables.async getVariables(): Promise<MaybeVariable[]> { const variables = await Promise.all( this.options.keys.map((key) => this.getVariable(key)), ); - return variables; + // Filter out null values to ensure only valid variables are returned + return variables.filter(Boolean); }packages/release-manager/test/variable-resolution.test.ts (1)
164-235: Context-specific release creation test is solid.
You might consider adding a test scenario where the variable value remains unchanged to confirm that no new release is triggered.packages/release-manager/src/rules/variable-rules.ts (3)
5-25: Clarify whether old vs new value differences are intended.
Although the class name isVariableChangedCondition, the current implementation only checks if the release trigger type isvariableand optionally matches on name/type. It doesn’t verify that the variable's new value differs from its previous value. If the intent is to confirm that the variable actually changed, you may want to evaluate old vs new values.
99-118: Consider using a structured logging approach instead of console.log.
Relying onconsole.logis convenient for simple debugging, but a structured logger (e.g., Winston, Pino, or similar) provides more control over levels, formatting, and log aggregation. This could improve maintainability and observability in production.
120-148: Handle potential concurrency or partial failures while updating dependent variables.
If multipleUpdateDependentVariablesActioninvocations occur concurrently for the same source variable, you may want to ensure that the underlying data store or operations are free from race conditions or partial updates.packages/release-manager/src/rules/version-rules.ts (4)
4-19: Verify actual version differences if needed.
This condition checks for a version-based trigger but doesn't differentiate whether the version changed from an old value to a new one. If you need to confirm an actual change occurred, consider also comparing old vs new version details.
41-104: Use a semver library for robust comparisons.
While the current implementation is fine for basic scenarios, a dedicated semver library (e.g.,semver) would provide a reliable, well-tested way to handle edge cases like pre-release versions, build metadata, and subtle patch/minor differences.
107-125: Logging approach.
Similar to the variable change logging, consider using a more feature-rich logger to manage levels, structures, and formatting in production environments.
142-175: Consider handling concurrency or partial deployments.
When multiple version changes happen in quick succession, confirm thattriggerDeploymentcan handle overlapping or repeated deployments without introducing conflicts or resource locks.packages/release-manager/test/release-manager.test.ts (3)
15-42: Add negative test scenarios.
The test ensures a release is created for a valid variable change. Consider adding tests for missing or invalid variable attributes (e.g. missing name) to verify robust error handling.
73-119: Consider additional coverage with multiple matching conditions.
The test checks that the rules run for a single condition and action. You might also add a scenario where multiple rules match simultaneously to ensure they each trigger their respective actions.
171-243: Integration workflow is comprehensive.
These end-to-end tests cover a realistic scenario. If concurrency or error paths must be tested (e.g. failing to log), consider extending this test suite to ensure reliability in adverse conditions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
packages/db/src/schema/deployment-variables.ts(2 hunks)packages/release-manager/README.md(1 hunks)packages/release-manager/eslint.config.js(1 hunks)packages/release-manager/package.json(1 hunks)packages/release-manager/src/db-variable-providers.tsx(1 hunks)packages/release-manager/src/index.ts(1 hunks)packages/release-manager/src/rules/index.ts(1 hunks)packages/release-manager/src/rules/variable-rules.ts(1 hunks)packages/release-manager/src/rules/version-rules.ts(1 hunks)packages/release-manager/src/types.ts(1 hunks)packages/release-manager/src/variables.ts(1 hunks)packages/release-manager/test/release-manager.test.ts(1 hunks)packages/release-manager/test/variable-resolution.test.ts(1 hunks)packages/release-manager/tsconfig.json(1 hunks)packages/release-manager/vitest.config.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/release-manager/src/rules/index.tspackages/release-manager/vitest.config.tspackages/release-manager/src/db-variable-providers.tsxpackages/release-manager/src/types.tspackages/release-manager/src/index.tspackages/release-manager/src/rules/variable-rules.tspackages/release-manager/src/rules/version-rules.tspackages/release-manager/test/release-manager.test.tspackages/release-manager/test/variable-resolution.test.tspackages/release-manager/src/variables.tspackages/db/src/schema/deployment-variables.ts
🧬 Code Definitions (4)
packages/release-manager/src/db-variable-providers.tsx (2)
packages/release-manager/src/types.ts (3)
VariableProvider(18-20)Variable(1-6)MaybeVariable(16-16)packages/db/src/schema/deployment-variables.ts (3)
DeploymentVariableValue(77-79)DeploymentVariable(47-47)deploymentVariable(23-45)
packages/release-manager/test/release-manager.test.ts (2)
packages/release-manager/src/rules/version-rules.ts (2)
VersionChangedCondition(4-19)LogVersionChangeAction(107-125)packages/release-manager/src/rules/variable-rules.ts (2)
VariableChangedCondition(5-25)LogVariableChangeAction(99-118)
packages/release-manager/src/variables.ts (1)
packages/release-manager/src/types.ts (2)
VariableProvider(18-20)MaybeVariable(16-16)
packages/db/src/schema/deployment-variables.ts (1)
packages/db/src/schema/deployment.ts (1)
deployment(63-87)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (29)
packages/release-manager/vitest.config.ts (1)
1-15: Well-structured Vitest configurationThe configuration is clean and follows good practices for a TypeScript testing setup. The test environment is properly set to "node", test file patterns are appropriately defined, and the path alias for "@" will help simplify import paths in test files.
packages/release-manager/src/rules/index.ts (1)
1-2: Good use of barrel exports patternThe index file correctly aggregates related rule modules, providing a clean interface for consumers to import rule-related functionality. This is a good pattern for organizing related modules.
packages/release-manager/tsconfig.json (1)
1-11: Solid TypeScript configurationThe TypeScript configuration extends a base config for consistency, properly sets up the output directory, and enables incremental builds for better development performance. The cache file location in node_modules/.cache keeps build artifacts separate from source code.
packages/release-manager/src/index.ts (1)
1-5: Well-organized package exportsThis index file provides a clean public API for the package by re-exporting functionality from core modules. The organization is logical, separating types, rules, releases, and storage concerns.
packages/release-manager/eslint.config.js (1)
1-13: Good ESLint configurationThe ESLint setup properly extends base configurations while providing package-specific overrides. The use of flat config format is forward-looking, and ignoring the dist directory is appropriate since it contains generated code.
The disabled rule
@typescript-eslint/require-awaitsuggests async functions without await statements are acceptable in this package. This aligns with the coding guidelines that indicate flexible error handling patterns are permitted.packages/release-manager/package.json (1)
1-38: Package configuration looks good!The package.json is well structured with appropriate dependencies and development tooling. The exports field correctly defines types and default JavaScript entry points. Scripts for building, testing, and linting are comprehensive.
I noticed that vitest uses a specific version (
^2.1.9) while other dependencies usecatalog:syntax. This is likely intentional but worth confirming for consistency.packages/release-manager/src/types.ts (3)
8-13: Well-structured Release typeThe Release type effectively captures the necessary components for managing releases.
15-16: Good utility types for flexibilityThese utility types provide a clean way to handle both synchronous and asynchronous operations while maintaining type safety.
18-20: Clear provider interfaceThe VariableProvider interface has a clean, focused API that will make it easy to implement different variable providers.
packages/release-manager/README.md (4)
1-15: Comprehensive documentation of featuresThe README provides a clear overview of the package's capabilities and features. Well-organized and informative.
25-34: Clear explanation of variable resolution hierarchyThe documentation clearly explains the priority order for variable resolution, which is crucial for users to understand how values are determined in different contexts.
37-83: Helpful usage example for context-specific releasesThis example effectively demonstrates how to create and use context-specific releases with the package.
139-186: Comprehensive rule engine exampleThe example demonstrates advanced functionality with the rule engine, showing how to create complex conditions and actions based on context.
packages/db/src/schema/deployment-variables.ts (4)
5-5: Import usage is correct.
The addition ofrelationsfromdrizzle-ormaligns well with the new relationship definitions.
119-135: New deploymentVariableRelationships appear valid.
References, fields, and expected one-to-one/many relationships match the schema definitions fordeploymentVariableanddeploymentVariableValue.
137-145: deploymentVariableValueRelationships correctly defines the single ownership with variable.
It accurately referencesdeploymentVariableValue.variableIdtodeploymentVariable.id.
147-159: deploymentVariableSetRelationships seems consistent.
The one-to-one references todeploymentandvariableSetare properly set with foreign keys.packages/release-manager/test/variable-resolution.test.ts (2)
13-162: Comprehensive test coverage for variable resolution.
Your test thoroughly verifies priority-based resolution across standard, deployment, and resource variables. The scenario setup and assertions look solid.
237-368: Context-specific rules processing is robustly tested.
Both production-specific and staging-specific rule conditions and actions look properly validated.packages/release-manager/src/db-variable-providers.tsx (3)
1-17: Initialization and import structure look fine.
No concerns with the introduced types or the optional DB transaction pattern.
19-47: DatabaseResourceVariableProvider’s lazy-load approach is correct.
Storing the promise inthis.variablesavoids redundant DB fetches effectively.
49-128: Deployment variable resolution logic is sound.
The for-loop evaluatingvaluesand fallback todefaultValuecovers different resource matching scenarios well.packages/release-manager/src/rules/variable-rules.ts (3)
27-45: Looks good.
This condition properly checks the trigger type, optional variable name, and a custom predicate. The logic is straightforward and appears correct.
47-81: Context checks are well-structured.
The code systematically checks whether each of the specified resource, environment, and deployment IDs matches the actual values. This is clear and maintainable.
83-96: Optional handling for partial label matches.
Currently, the condition requires an exact match on a single label key-value pair. If partial matches or multiple label checks become necessary, you may want to introduce more flexible matching strategies. Otherwise, the current direct equality check is fine.packages/release-manager/src/rules/version-rules.ts (2)
21-39: Predicate-based check looks good.
This satisfies a straightforward approach for running a custom predicate on the version string, with an optionalversionIdcheck. Nicely done.
127-140: Callback-based action.
TheMajorVersionChangeActionis straightforward and focuses on the callback logic. Ensure the callback function handles errors gracefully in its own implementation if long-running or asynchronous operations are involved.packages/release-manager/test/release-manager.test.ts (2)
44-70: Release creation for version change is verified thoroughly.
This test properly asserts release creation and idempotency. The coverage here looks solid.
120-169: Selective action execution confirmed.
It’s good to see that only the relevant action is executed for the appropriate trigger. This test effectively checks condition discrimination.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/release-manager/src/variables.ts (1)
20-26: Consider adding error handling to getVariable methodThe current implementation doesn't handle exceptions from variable providers. If a provider throws an error, it will interrupt the entire variable resolution process.
async getVariable(key: string): Promise<MaybeVariable> { for (const provider of this.variableProviders) { - const variable = await provider.getVariable(key); - if (variable) return variable; + try { + const variable = await provider.getVariable(key); + if (variable) return variable; + } catch (error) { + console.error(`Error retrieving variable '${key}' from provider:`, error); + // Continue to next provider instead of failing + } } return null; }packages/release-manager/src/db-variable-providers.tsx (5)
1-1: File extension should be.tsinstead of.tsxThis file doesn't contain any JSX or React components, so it should have a
.tsextension instead of.tsx.Consider renaming the file to
db-variable-providers.tsfor consistency and clarity.
76-84: Missingasynckeyword inloadVariablesmethodThe
loadVariablesmethod returns a Promise but isn't marked asasync. While this works because the method returns a Promise directly, adding theasynckeyword would maintain consistency with theloadVariablesmethod in theDatabaseResourceVariableProviderclass.- private loadVariables() { + private async loadVariables() { return this.db.query.deploymentVariable.findMany({ where: eq(deploymentVariable.deploymentId, this.options.deploymentId), with: { defaultValue: true, values: true, }, }); }
90-125: Consider optimizing resource matching ingetVariableThe current implementation performs a separate database query for each value in the
valuesarray. This could be inefficient if there are many values, especially since you're querying the same resource multiple times with different selectors.Consider pre-fetching the resource once and then matching against its metadata in memory, or using a more efficient query pattern that can check multiple selectors in a single database call.
async getVariable(key: string): Promise<MaybeVariable> { const variables = await this.getVariables(); const variable = variables.find((v) => v.key === key) ?? null; if (variable == null) return null; + // Fetch the resource once + const resource = await this.db + .select() + .from(resource) + .where(eq(resource.id, this.options.resourceId)) + .then(takeFirstOrNull); + + if (resource == null) return null; + for (const value of variable.values) { - const res = await this.db - .select() - .from(resource) - .where( - and( - eq(resource.id, this.options.resourceId), - resourceMatchesMetadata(this.db, value.resourceSelector), - ), - ) - .then(takeFirstOrNull); - - if (res != null) + // Check if the resource matches the selector in memory or with a helper function + const matches = resourceMatchesMetadataInMemory(resource, value.resourceSelector); + if (matches) return { id: variable.id, key, sensitive: false, ...value, }; } // Rest of the method remains the sameNote: You would need to implement the
resourceMatchesMetadataInMemoryfunction to match the behavior ofresourceMatchesMetadata.
56-66: Consider using database schema types instead of local definitionsThe locally defined
DeploymentVariableValueandDeploymentVariabletypes appear to duplicate types from the database schema. Consider importing and using the schema types directly to maintain consistency and avoid duplication.-type DeploymentVariableValue = { - value: any; - resourceSelector: any; -}; - -type DeploymentVariable = { - id: string; - key: string; - defaultValue: DeploymentVariableValue | null; - values: DeploymentVariableValue[]; -}; +import { DeploymentVariable, DeploymentVariableValue } from "@ctrlplane/db/schema/deployment-variables"; + +// Add type extensions or transformations if needed +type ExtendedDeploymentVariable = DeploymentVariable & { + defaultValue: DeploymentVariableValue | null; + values: DeploymentVariableValue[]; +};
86-88: Inconsistent method namingThe method is named
getVariableshere but the equivalent method inDatabaseResourceVariableProvideris namedgetVariablesPromise. Consider using consistent method names for similar functionality.- private getVariables() { + private getVariablesPromise() { return (this.variables ??= this.loadVariables()); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/release-manager/src/db-variable-providers.tsx(1 hunks)packages/release-manager/src/index.ts(1 hunks)packages/release-manager/src/variables.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/release-manager/src/index.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/release-manager/src/variables.tspackages/release-manager/src/db-variable-providers.tsx
🧬 Code Definitions (2)
packages/release-manager/src/variables.ts (1)
packages/release-manager/src/types.ts (2)
VariableProvider(18-20)MaybeVariable(16-16)
packages/release-manager/src/db-variable-providers.tsx (2)
packages/release-manager/src/types.ts (3)
VariableProvider(18-20)Variable(1-6)MaybeVariable(16-16)packages/db/src/schema/deployment-variables.ts (3)
DeploymentVariableValue(77-79)DeploymentVariable(47-47)deploymentVariable(23-45)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (4)
packages/release-manager/src/variables.ts (2)
3-8: ReleaseManagerOptions type should be exportedThe
ReleaseManagerOptionstype is used by theVariableManagerclass but isn't exported, making it unavailable to consumers who might need to construct these options.-type ReleaseManagerOptions = { +export type ReleaseManagerOptions = { deploymentId: string; resourceId: string; keys: string[]; };
16-18: LGTM! Efficient parallel variable retrievalThe implementation of
getVariablesmethod usingPromise.allis efficient as it allows for parallel retrieval of all variables, reducing overall latency.packages/release-manager/src/db-variable-providers.tsx (2)
39-41: Nice caching implementation with memoizationThe use of the nullish coalescing assignment operator to memoize the variables promise is a clean and efficient approach to caching.
27-37: Good approach for extracting and mapping database resultsThe implementation correctly extracts and maps the database results to the expected Variable type, ensuring a clean interface.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/release-manager/src/db-variable-providers.ts (3)
55-65: Consider using more specific type forvalueandresourceSelectorInstead of using
anytype, consider defining more specific types for these properties to improve type safety.type DeploymentVariableValue = { - value: any; - resourceSelector: any; + value: unknown; + resourceSelector: Record<string, unknown>; };
75-87: Inconsistent async/await usage compared to other providersUnlike
loadVariablesinDatabaseResourceVariableProvider, this method isn't marked asasyncdespite returning a Promise. While this works, it creates an inconsistency between similar classes.- private loadVariables() { + private async loadVariables() { return this.db.query.deploymentVariable.findMany({ where: eq(deploymentVariable.deploymentId, this.options.deploymentId), with: { defaultValue: true, values: true, }, }); } - private getVariables() { + private getVariables() { return (this.variables ??= this.loadVariables()); }
132-142: Missing implementation documentation for placeholder providerThe
DatabaseSystemVariableSetProvideralways returns null, which suggests it's a placeholder. Consider adding a comment or TODO to indicate its intended purpose or if it's pending implementation.export class DatabaseSystemVariableSetProvider implements VariableProvider { private db: Tx; constructor(private options: DatabaseSystemVariableSetOptions) { this.db = options.db ?? db; } + // TODO: Implement variable retrieval for system variable sets getVariable(_: string): MaybeVariable { return null; } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/release-manager/src/db-variable-providers.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/release-manager/src/db-variable-providers.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (1)
packages/release-manager/src/db-variable-providers.ts (1)
19-47: LGTM: Well-structured variable provider implementationThe implementation follows good practices with lazy loading of variables and proper interface implementation.
| async getVariable(key: string): Promise<MaybeVariable> { | ||
| const variables = await this.getVariables(); | ||
| const variable = variables.find((v) => v.key === key) ?? null; | ||
| if (variable == null) return null; | ||
|
|
||
| for (const value of variable.values) { | ||
| const res = await this.db | ||
| .select() | ||
| .from(resource) | ||
| .where( | ||
| and( | ||
| eq(resource.id, this.options.resourceId), | ||
| resourceMatchesMetadata(this.db, value.resourceSelector), | ||
| ), | ||
| ) | ||
| .then(takeFirstOrNull); | ||
|
|
||
| if (res != null) | ||
| return { | ||
| id: variable.id, | ||
| key, | ||
| sensitive: false, | ||
| ...value, | ||
| }; | ||
| } | ||
|
|
||
| if (variable.defaultValue != null) | ||
| return { | ||
| id: variable.id, | ||
| key, | ||
| sensitive: false, | ||
| ...variable.defaultValue, | ||
| }; | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hardcoded sensitive property and potential object spread issues
The method sets sensitive: false for all variables, which might not be accurate if some variables should be considered sensitive. Additionally, using the spread operator with value and variable.defaultValue could potentially override explicitly set properties if they contain keys like id, key, or sensitive.
if (res != null)
return {
id: variable.id,
key,
sensitive: false,
- ...value,
+ value: value.value,
+ resourceSelector: value.resourceSelector,
};
if (variable.defaultValue != null)
return {
id: variable.id,
key,
sensitive: false,
- ...variable.defaultValue,
+ value: variable.defaultValue.value,
+ resourceSelector: variable.defaultValue.resourceSelector,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async getVariable(key: string): Promise<MaybeVariable> { | |
| const variables = await this.getVariables(); | |
| const variable = variables.find((v) => v.key === key) ?? null; | |
| if (variable == null) return null; | |
| for (const value of variable.values) { | |
| const res = await this.db | |
| .select() | |
| .from(resource) | |
| .where( | |
| and( | |
| eq(resource.id, this.options.resourceId), | |
| resourceMatchesMetadata(this.db, value.resourceSelector), | |
| ), | |
| ) | |
| .then(takeFirstOrNull); | |
| if (res != null) | |
| return { | |
| id: variable.id, | |
| key, | |
| sensitive: false, | |
| ...value, | |
| }; | |
| } | |
| if (variable.defaultValue != null) | |
| return { | |
| id: variable.id, | |
| key, | |
| sensitive: false, | |
| ...variable.defaultValue, | |
| }; | |
| return null; | |
| } | |
| async getVariable(key: string): Promise<MaybeVariable> { | |
| const variables = await this.getVariables(); | |
| const variable = variables.find((v) => v.key === key) ?? null; | |
| if (variable == null) return null; | |
| for (const value of variable.values) { | |
| const res = await this.db | |
| .select() | |
| .from(resource) | |
| .where( | |
| and( | |
| eq(resource.id, this.options.resourceId), | |
| resourceMatchesMetadata(this.db, value.resourceSelector), | |
| ), | |
| ) | |
| .then(takeFirstOrNull); | |
| if (res != null) | |
| return { | |
| id: variable.id, | |
| key, | |
| sensitive: false, | |
| value: value.value, | |
| resourceSelector: value.resourceSelector, | |
| }; | |
| } | |
| if (variable.defaultValue != null) | |
| return { | |
| id: variable.id, | |
| key, | |
| sensitive: false, | |
| value: variable.defaultValue.value, | |
| resourceSelector: variable.defaultValue.resourceSelector, | |
| }; | |
| return null; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/release-manager/src/db-variable-providers.ts (4)
59-62: Consider using more specific types instead ofanyUsing
anyforvalueandresourceSelectorproperties reduces type safety. Consider defining more specific types based on the expected structure of these values to improve code maintainability and catch potential errors at compile time.type DeploymentVariableValue = { - value: any; - resourceSelector: any; + value: Record<string, unknown>; + resourceSelector: Record<string, unknown>; };
167-177: Hardcodedsensitiveflag in SystemVariableSetProviderSimilar to the issue in
DatabaseDeploymentVariableProvider, this method also setssensitive: falsefor all variables, which might not be accurate. Consider deriving this value from the variable itself if such information is available.async getVariable(key: string): Promise<MaybeVariable> { const variables = await this.getVariables(); const variable = variables.find((v) => v.key === key) ?? null; if (variable == null) return null; return { id: variable.id, key, value: variable.value, - sensitive: false, + sensitive: variable.sensitive ?? false, // Use variable's sensitive property if available }; }
31-41: Consider adding error handling for database operationsWhile the coding guidelines don't strictly enforce try/catch blocks, adding error handling for database operations would improve robustness. These methods fetch data but don't handle potential database errors, which could lead to unhandled promise rejections.
For each loadVariables method, consider adding error handling or at least logging:
private async loadVariables() { try { const variables = await this.db.query.resourceVariable.findMany({ where: and(eq(resourceVariable.resourceId, this.options.resourceId)), }); return variables.map((v) => ({ id: v.id, key: v.key, value: v.value, sensitive: v.sensitive, })); } catch (error) { console.error("Failed to load resource variables:", error); // Either rethrow or return empty array depending on requirements return []; } }Also applies to: 79-87, 145-161
132-133: Commented out systemId parameterThere's a commented out parameter
systemIdin the options type. If this is planned for future implementation, consider adding a TODO comment explaining why it's commented out and when it will be used. Otherwise, it might be better to remove it entirely.export type DatabaseSystemVariableSetOptions = { - // systemId: string; + // TODO: Add systemId parameter when system-level variable sets are implemented environmentId: string; db?: Tx; };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/release-manager/src/db-variable-providers.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/release-manager/src/db-variable-providers.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (2)
packages/release-manager/src/db-variable-providers.ts (2)
110-117: Hardcodedsensitiveproperty and potential object spread issuesThe method sets
sensitive: falsefor all variables, which might not be accurate if some variables should be considered sensitive. Additionally, using the spread operator withvaluecould potentially override explicitly set properties if they contain keys likeid,key, orsensitive.if (res != null) return { id: variable.id, key, sensitive: false, - ...value, + value: value.value, + resourceSelector: value.resourceSelector, };
119-126: Similar spread operator issue with defaultValueUsing the spread operator with
variable.defaultValuecould potentially override explicitly set properties if they contain keys likeid,key, orsensitive.if (variable.defaultValue != null) return { id: variable.id, key, sensitive: false, - ...variable.defaultValue, + value: variable.defaultValue.value, + resourceSelector: variable.defaultValue.resourceSelector, };
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/release-manager/src/releases.ts (2)
2-2: Consider using named imports from lodash.If you only need a handful of utilities (like
isEqual), importing the entire lodash library may increase bundle size unnecessarily.- import * as _ from "lodash"; + import isEqual from "lodash/isEqual";
61-107: Consider concurrency and uniqueness constraints for your releases.Multiple concurrent calls to
createReleasecould insert duplicate releases for the same version and environment. You might want to enforce a unique constraint or ensure a transactional check to avoid duplicate records.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/release-manager/package.json(1 hunks)packages/release-manager/src/releases.ts(1 hunks)packages/release-manager/src/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/release-manager/package.json
- packages/release-manager/src/types.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/release-manager/src/releases.ts
🧬 Code Definitions (1)
packages/release-manager/src/releases.ts (1)
packages/release-manager/src/types.ts (3)
Release(8-14)MaybeVariable(17-17)Variable(1-6)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (2)
packages/release-manager/src/releases.ts (2)
10-14: Well-defined options interface.These fields sufficiently capture the essential context for releases.
16-26: The ReleaseManager interface looks good.The definitions for core methods—
getLatestRelease(),createRelease(), andensureRelease()—align well with typical release management requirements.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/release-manager/src/variables.ts (1)
14-20: 🛠️ Refactor suggestionExport VariableManagerOptions type.
The VariableManagerOptions type is used by the VariableManager class but isn't exported, making it unavailable to consumers who might need to construct these options.
-type VariableManagerOptions = { +export type VariableManagerOptions = { deploymentId: string; resourceId: string; environmentId: string; keys: string[]; };packages/release-manager/src/releases.ts (2)
39-42:⚠️ Potential issueFix type mismatch between interface and implementation.
The ReleaseCreator interface declares
ensureRelease(versionId: string, variables: MaybeVariable[]), but it's implemented as(versionId: string, variables: Variable[]). This causes a type conflict.async ensureRelease( versionId: string, - variables: Variable[], + variables: MaybeVariable[], ): Promise<ReleaseWithId> {
45-55: 🛠️ Refactor suggestionImprove variable comparison logic.
The comparison only considers variable keys and values, ignoring other properties like
idandsensitive. This could lead to incorrect release equality detection.const latestR = { versionId: latestRelease?.versionId, variables: Object.fromEntries( - latestRelease?.variables.map((v) => [v.key, v.value]) ?? [], + latestRelease?.variables.map((v) => [v.key, { + value: v.value, + sensitive: v.sensitive, + id: v.id + }]) ?? [], ), }; const newR = { versionId, - variables: Object.fromEntries(variables.map((v) => [v.key, v.value])), + variables: Object.fromEntries(variables.map((v) => [v.key, { + value: v.value, + sensitive: v.sensitive, + id: v.id + }])), };
🧹 Nitpick comments (5)
packages/release-manager/src/manager.ts (2)
19-28: Add JSDoc comments to document the ReleaseManager class.The ReleaseManager class appears to be a central component in the release management system, but lacks documentation explaining its purpose and usage patterns.
+/** + * Manages releases for a specific environment, deployment, and resource. + * Provides functionality to retrieve variables, ensure releases exist, + * and set desired release targets. + */ export class ReleaseManager {
30-35: Consider adding error handling for variable manager initialization.The getCurrentVariables method initializes the variableManager if null, but doesn't handle potential errors during initialization. Consider adding try/catch or returning a more detailed error.
async getCurrentVariables() { - if (this.variableManager === null) - this.variableManager = await VariableManager.database(this.options); + if (this.variableManager === null) { + try { + this.variableManager = await VariableManager.database(this.options); + } catch (error) { + throw new Error(`Failed to initialize variable manager: ${error.message}`); + } + } return this.variableManager.getVariables(); }packages/release-manager/src/variables.ts (2)
26-43: Document the database method with JSDoc.The static database method is a factory method that provides important initialization logic, but lacks documentation about its purpose and behavior.
export class VariableManager { + /** + * Creates a VariableManager instance with database-backed variable providers. + * Retrieves the list of variable keys from the deployment variable table. + * + * @param options Configuration options including deployment, resource, and environment IDs + * @returns A new VariableManager instance initialized with database providers + */ static async database(options: DatabaseVariableManagerOptions) {
54-60: Consider adding logging for variable resolution.The getVariable method silently returns null when no providers can resolve a variable. Consider adding logging to help debug missing variables.
async getVariable(key: string): Promise<MaybeVariable> { for (const provider of this.variableProviders) { const variable = await provider.getVariable(key); if (variable) return variable; } + // Consider logging that no provider found this variable return null; }packages/release-manager/src/releases.ts (1)
86-108: Add error handling for database operations.The createRelease method doesn't handle potential database errors. Consider adding error handling to provide more meaningful error messages.
async createRelease( versionId: string, variables: Variable[], ): Promise<ReleaseWithId> { const release: Release = { resourceId: this.options.resourceId, deploymentId: this.options.deploymentId, environmentId: this.options.environmentId, versionId, variables, }; - const dbRelease = await this.db - .insert(schema.release) - .values(release) - .returning() - .then(takeFirst); + try { + const dbRelease = await this.db + .insert(schema.release) + .values(release) + .returning() + .then(takeFirst); + + return { + ...release, + ...dbRelease, + }; + } catch (error) { + throw new Error(`Failed to create release: ${error.message}`); + } - - return { - ...release, - ...dbRelease, - }; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/release-manager/src/index.ts(1 hunks)packages/release-manager/src/manager.ts(1 hunks)packages/release-manager/src/releases.ts(1 hunks)packages/release-manager/src/variables.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/release-manager/src/index.tspackages/release-manager/src/manager.tspackages/release-manager/src/variables.tspackages/release-manager/src/releases.ts
🧬 Code Definitions (1)
packages/release-manager/src/releases.ts (2)
packages/release-manager/src/manager.ts (1)
ReleaseManagerOptions(11-17)packages/release-manager/src/types.ts (3)
Release(8-14)MaybeVariable(17-17)Variable(1-6)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (3)
packages/release-manager/src/index.ts (1)
1-3: LGTM - Clean export pattern for package entry point.The file correctly re-exports all entities from the core modules, providing a clean API surface for the package.
packages/release-manager/src/manager.ts (2)
11-17: LGTM - Well-defined options type.The ReleaseManagerOptions type is well-structured with clear, descriptive field names.
47-68:❌ Incorrect review comment
Consider validating the existence of the release ID.
The setDesiredRelease method doesn't verify if the provided releaseId actually exists before setting it as the desired release.
Run the following script to check if the code validates release existence elsewhere:
🏁 Script executed:
#!/bin/bash # Check if there's validation for release existence elsewhere rg -A 5 -B 5 "setDesiredRelease|desiredReleaseId" --type tsLength of output: 7492
Validation Already Performed Upstream
After verifying the codebase, it appears that validating the existence of a release ID is handled before calling
setDesiredRelease. The release ID is ensured (viareleaseCreator.ensureReleaseand the logic ingetDesiredin the rule engine) before being passed to this method, so no additional validation is necessary withinsetDesiredRelease.Likely an incorrect or invalid review comment.
| type ReleaseManagerOptions = { | ||
| environmentId: string; | ||
| deploymentId: string; | ||
| resourceId: string; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Export ReleaseManagerOptions type.
The ReleaseManagerOptions type is defined twice - once here and once in manager.ts (line 11). Consider centralizing this definition to avoid duplication.
-type ReleaseManagerOptions = {
+export type ReleaseManagerOptions = {
environmentId: string;
deploymentId: string;
resourceId: string;
};And then import it in manager.ts instead of redefining it.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type ReleaseManagerOptions = { | |
| environmentId: string; | |
| deploymentId: string; | |
| resourceId: string; | |
| }; | |
| export type ReleaseManagerOptions = { | |
| environmentId: string; | |
| deploymentId: string; | |
| resourceId: string; | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/release-manager/src/releases.ts (1)
48-70: 🛠️ Refactor suggestionRevisit variable comparison logic.
The comparison only considers
keyandvalueproperties, ignoring other variable properties likesensitive. If these properties impact a release, you might incorrectly detect two releases as identical.- const latestR = { - versionId: latestRelease?.versionId, - variables: Object.fromEntries( - latestRelease?.variables.map((v) => [v.key, v.value]) ?? [], - ), - }; - - const newR = { - versionId, - variables: Object.fromEntries(nonNullVariables.map((v) => [v.key, v.value])), - }; + const latestR = { + versionId: latestRelease?.versionId, + variables: Object.fromEntries( + latestRelease?.variables.map((v) => [v.key, { + value: v.value, + sensitive: v.sensitive, + // Include other relevant properties + }]) ?? [], + ), + }; + + const newR = { + versionId, + variables: Object.fromEntries(nonNullVariables.map((v) => [v.key, { + value: v.value, + sensitive: v.sensitive, + // Include other relevant properties + }])), + };
🧹 Nitpick comments (5)
packages/release-manager/src/manager.ts (1)
26-34: Consider memoizing variables.The
getCurrentVariablesmethod initializes thevariableManagerwhen null, which is good. However, it always callsgetVariables()which might be expensive. Consider caching the variables if they're unlikely to change during the lifetime of this instance.- async getCurrentVariables() { + private variablesCache: MaybeVariable[] | null = null; + + async getCurrentVariables() { if (this.variableManager === null) this.variableManager = await VariableManager.database({ ...this.options, db: this.db, }); - - return this.variableManager.getVariables(); + + if (this.variablesCache === null) { + this.variablesCache = await this.variableManager.getVariables(); + } + return this.variablesCache; }packages/release-manager/src/providers/variable-provider-factories.ts (1)
31-45: Consider adding duplicate prevention in registry.The implementation of the
DefaultVariableProviderRegistryclass is simple and clean. However, theregistermethod doesn't prevent duplicate factories from being registered, which could lead to redundant processing. Consider adding a check for duplicates if that's a concern in your use case.register(factory: VariableProviderFactory) { + // Add check to prevent duplicate factories if necessary + if (!this.providers.some(p => p.constructor === factory.constructor)) { this.providers.push(factory); + } }packages/release-manager/src/variables.ts (1)
11-22: Database-based initialization is well-structured.Fetching keys with
getDeploymentVariableKeysand creating providers via a registry is consistent. Consider adding error-handling if the key retrieval fails, but it's not mandatory.packages/release-manager/src/types.ts (2)
1-6: Variable type is well-structured.It captures essential fields (id, key, value, sensitivity). The default
anytype forvalueis flexible but may limit type safety.
27-32: Flexible VariableProviderOptions.The
db?: anyproperty trades type safety for flexibility. Consider narrowing the type if possible.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/release-manager/src/index.ts(1 hunks)packages/release-manager/src/manager.ts(1 hunks)packages/release-manager/src/providers/variable-provider-factories.ts(1 hunks)packages/release-manager/src/releases.ts(1 hunks)packages/release-manager/src/repositories/release-repository.ts(1 hunks)packages/release-manager/src/types.ts(1 hunks)packages/release-manager/src/variables.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/release-manager/src/repositories/release-repository.tspackages/release-manager/src/manager.tspackages/release-manager/src/providers/variable-provider-factories.tspackages/release-manager/src/releases.tspackages/release-manager/src/variables.tspackages/release-manager/src/index.tspackages/release-manager/src/types.ts
🧬 Code Definitions (5)
packages/release-manager/src/repositories/release-repository.ts (1)
packages/release-manager/src/types.ts (3)
ReleaseRepository(34-38)ReleaseQueryOptions(40-44)Release(8-14)
packages/release-manager/src/manager.ts (4)
packages/release-manager/src/types.ts (1)
ReleaseQueryOptions(40-44)packages/release-manager/src/releases.ts (1)
BaseReleaseCreator(20-71)packages/release-manager/src/variables.ts (1)
VariableManager(11-49)packages/release-manager/src/repositories/release-repository.ts (1)
DatabaseReleaseRepository(8-62)
packages/release-manager/src/providers/variable-provider-factories.ts (1)
packages/release-manager/src/types.ts (2)
VariableProviderFactory(23-25)VariableProviderOptions(27-32)
packages/release-manager/src/releases.ts (2)
packages/release-manager/src/types.ts (4)
Release(8-14)MaybeVariable(17-17)ReleaseQueryOptions(40-44)ReleaseRepository(34-38)packages/release-manager/src/repositories/release-repository.ts (1)
DatabaseReleaseRepository(8-62)
packages/release-manager/src/variables.ts (2)
packages/release-manager/src/types.ts (3)
VariableProviderOptions(27-32)VariableProvider(19-21)MaybeVariable(17-17)packages/release-manager/src/providers/variable-provider-factories.ts (2)
getDeploymentVariableKeys(47-54)DefaultVariableProviderRegistry(31-45)
🪛 Biome (1.9.4)
packages/release-manager/src/repositories/release-repository.ts
[error] 9-9: This parameter is used before its declaration.
The parameter is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (30)
packages/release-manager/src/index.ts (1)
1-6: Looks good - clean module exports structure.The file correctly exports all the necessary components from the package, providing a clean entry point for consumers.
packages/release-manager/src/manager.ts (4)
9-11: LGTM: Clean type definition.The
ReleaseManagerOptionstype properly extendsReleaseQueryOptionswith an optional database transaction parameter.
13-24: Dependency initialization looks good.The
ReleaseManagerclass properly initializes its dependencies, with a good pattern of fallback to the default database connection when no transaction is provided.
36-44: LGTM: Well-structured method with good conditional logic.The
ensureReleasemethod has a clean implementation with a good pattern for optional parameters and conditional behavior.
46-51: LGTM: Clean delegation to repository.The
setDesiredReleasemethod correctly delegates to the repository's implementation, passing all necessary parameters.packages/release-manager/src/releases.ts (5)
6-6: LGTM: Clean type definition.The
ReleaseWithIdtype correctly extends the baseReleasetype with an id field.
8-18: LGTM: Well-defined interface.The
ReleaseCreatorinterface clearly defines the contract for release creation.
20-28: LGTM: Good constructor and repository setup.The class constructor and repository setup look good. The fluent interface pattern with
return thisinsetRepositoryis a nice touch.
30-32: LGTM: Simple delegation.The
getLatestReleasemethod correctly delegates to the repository.
34-46: LGTM: Good null filtering.The
createReleasemethod efficiently filters out null variables and properly creates a release with the necessary properties.packages/release-manager/src/providers/variable-provider-factories.ts (4)
13-17: LGTM: Simple factory implementation.The
ResourceVariableProviderFactorycorrectly implements theVariableProviderFactoryinterface with a clean implementation.
19-23: LGTM: Simple factory implementation.The
DeploymentVariableProviderFactorycorrectly implements theVariableProviderFactoryinterface with a clean implementation.
25-29: LGTM: Simple factory implementation.The
SystemVariableSetProviderFactorycorrectly implements theVariableProviderFactoryinterface with a clean implementation.
47-54: LGTM: Clean database query implementation.The
getDeploymentVariableKeysfunction has a clean implementation with proper fallback to the default database connection when no transaction is provided.packages/release-manager/src/repositories/release-repository.ts (5)
1-4: Imports look good.No issues with these imports. They provide the necessary types and utility functions cleanly.
8-9: Static analysis hint likely a false positive.The
noInvalidUseBeforeDeclarationwarning for line 9 may be a false positive. Referencing the globally importeddbas the default value in the constructor parameter is valid. If your environment flags this usage, consider assigningdbto a local variable before passing it as a default.🧰 Tools
🪛 Biome (1.9.4)
[error] 9-9: This parameter is used before its declaration.
The parameter is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
11-25: Query for the latest release appears solid.This method correctly finds the latest release or returns
nullif none is found. Ensure indices on relevant columns (resourceId,deploymentId,environmentId) exist for performance if large datasets are expected.
27-38: Creation logic is straightforward.Inserting a release and merging returned data provides a predictable and consistent dataset. No issues found.
40-61: Conflict resolution approach is appropriate.Using
onConflictDoUpdatewithbuildConflictUpdateColumnsproperly updates only thedesiredReleaseId. This ensures minimal data overwriting. No concurrency or correctness issues are apparent.packages/release-manager/src/variables.ts (5)
1-5: Imports are concise and relevant.All imports serve clear purposes, referencing types, factories, and registries.
7-9: Type definition for VariableManagerOptions is clear.The additional
keysproperty extendsVariableProviderOptionsnicely. No concerns here.
24-27: Constructor design is straightforward.Storing the options and providers in private properties is a neat approach.
29-36: Provider management methods are extensible.
getProviders()andaddProvider()effectively manage the list. No issues identified.
38-48: Variable fetching logic is sound.Parallel retrieval via
Promise.allis adequate ingetVariables(), and short-circuiting ingetVariable()is efficient.packages/release-manager/src/types.ts (6)
8-14: Release type covers necessary fields.This aligns with standard release tracking, associating variables with their IDs.
16-17: Utility types are neat.
MaybePromiseandMaybeVariableare concise and improve code clarity.
19-21: VariableProvider interface is straightforward.Defines a single
getVariablemethod, cleanly typed.
23-25: Factory interface is concise.
create(options)returning aVariableProvideris clearly defined.
34-38: ReleaseRepository interface is properly scoped.It defines all essential CRUD operations for release entities.
40-44: ReleaseQueryOptions is specific and unambiguous.This type eliminates confusion when querying releases.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
packages/release-manager/src/providers/variable-provider-factories.ts (3)
6-10: Consider using a more compact import format.The import statement is unnecessarily spread across multiple lines. Consider using a more concise format for better readability:
-import { - DatabaseDeploymentVariableProvider, - DatabaseResourceVariableProvider, - DatabaseSystemVariableSetProvider -} from "../db-variable-providers.js"; +import { DatabaseDeploymentVariableProvider, DatabaseResourceVariableProvider, DatabaseSystemVariableSetProvider } from "../db-variable-providers.js";
31-45: Consider adding an interface for the registry.The
DefaultVariableProviderRegistryclass doesn't implement an interface, which might make it harder to mock or replace in tests. Additionally, consider adding explicit access modifiers to the methods for better clarity.+export interface VariableProviderRegistry { + register(factory: VariableProviderFactory): void; + getFactories(): VariableProviderFactory[]; +} -export class DefaultVariableProviderRegistry { +export class DefaultVariableProviderRegistry implements VariableProviderRegistry { private providers: VariableProviderFactory[] = [ new ResourceVariableProviderFactory(), new DeploymentVariableProviderFactory(), new SystemVariableSetProviderFactory(), ]; - register(factory: VariableProviderFactory) { + public register(factory: VariableProviderFactory): void { this.providers.push(factory); } - getFactories() { + public getFactories(): VariableProviderFactory[] { return [...this.providers]; } }
47-54: Consider adding error handling to the database query.The
getDeploymentVariableKeysfunction performs a database operation without explicit error handling. Consider adding try/catch to handle potential database errors gracefully.export async function getDeploymentVariableKeys(options: Pick<ReleaseIdentifier, 'deploymentId'> & { db?: Tx }): Promise<string[]> { const tx = options.db ?? db; + try { return tx .select({ key: schema.deploymentVariable.key }) .from(schema.deploymentVariable) .where(eq(schema.deploymentVariable.deploymentId, options.deploymentId)) .then((results) => results.map((r) => r.key)); + } catch (error) { + console.error(`Failed to get deployment variable keys for deployment ${options.deploymentId}:`, error); + return []; + } }packages/release-manager/src/db-variable-providers.ts (4)
30-33: Remove unnecessaryandoperator with a single condition.The
andoperator is unnecessary when there's only a single condition:private async loadVariables() { const variables = await this.db.query.resourceVariable.findMany({ - where: and(eq(resourceVariable.resourceId, this.options.resourceId)), + where: eq(resourceVariable.resourceId, this.options.resourceId), }); return variables.map((v) => ({ id: v.id, key: v.key, value: v.value, sensitive: v.sensitive, })); }
18-20: Consider consolidating similar option types.The three option types
DatabaseResourceVariableOptions,DatabaseDeploymentVariableOptions, andDatabaseSystemVariableSetOptionshave similar structure. Consider creating a base type and extending it for each specific use case:+export type DatabaseVariableProviderBaseOptions = { + db?: Tx; +}; -export type DatabaseResourceVariableOptions = Pick<ReleaseIdentifier, 'resourceId'> & { - db?: Tx; -}; +export type DatabaseResourceVariableOptions = Pick<ReleaseIdentifier, 'resourceId'> & DatabaseVariableProviderBaseOptions; -export type DatabaseDeploymentVariableOptions = Pick<ReleaseIdentifier, 'resourceId' | 'deploymentId'> & { - db?: Tx; -}; +export type DatabaseDeploymentVariableOptions = Pick<ReleaseIdentifier, 'resourceId' | 'deploymentId'> & DatabaseVariableProviderBaseOptions; -export type DatabaseSystemVariableSetOptions = Pick<ReleaseIdentifier, 'environmentId'> & { - db?: Tx; -}; +export type DatabaseSystemVariableSetOptions = Pick<ReleaseIdentifier, 'environmentId'> & DatabaseVariableProviderBaseOptions;Also applies to: 52-54, 128-130
56-59: Consider improving type specificity forresourceSelector.The
valueandresourceSelectorproperties are typed asany, which loses type safety. Consider using more specific types if possible:type DeploymentVariableValue = { - value: any; - resourceSelector: any; + value: string | number | boolean | object; + resourceSelector: Record<string, unknown>; };
76-84: Consider adding error handling to the database query.The
loadVariablesmethod performs a database operation without explicit error handling. Consider adding try/catch to handle potential database errors gracefully:private loadVariables() { + try { return this.db.query.deploymentVariable.findMany({ where: eq(deploymentVariable.deploymentId, this.options.deploymentId), with: { defaultValue: true, values: { orderBy: [asc(deploymentVariableValue.value)] }, }, }); + } catch (error) { + console.error(`Failed to load deployment variables for deployment ${this.options.deploymentId}:`, error); + return []; + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/release-manager/src/db-variable-providers.ts(1 hunks)packages/release-manager/src/manager.ts(1 hunks)packages/release-manager/src/providers/variable-provider-factories.ts(1 hunks)packages/release-manager/src/releases.ts(1 hunks)packages/release-manager/src/repositories/release-repository.ts(1 hunks)packages/release-manager/src/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/release-manager/src/manager.ts
- packages/release-manager/src/types.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/release-manager/src/releases.tspackages/release-manager/src/db-variable-providers.tspackages/release-manager/src/providers/variable-provider-factories.tspackages/release-manager/src/repositories/release-repository.ts
🧬 Code Definitions (4)
packages/release-manager/src/releases.ts (2)
packages/release-manager/src/types.ts (4)
Release(14-17)MaybeVariable(20-20)ReleaseIdentifier(1-5)ReleaseRepository(34-38)packages/release-manager/src/repositories/release-repository.ts (1)
DatabaseReleaseRepository(8-62)
packages/release-manager/src/db-variable-providers.ts (1)
packages/release-manager/src/types.ts (4)
ReleaseIdentifier(1-5)VariableProvider(22-24)Variable(7-12)MaybeVariable(20-20)
packages/release-manager/src/providers/variable-provider-factories.ts (2)
packages/release-manager/src/types.ts (3)
VariableProviderFactory(26-28)VariableProviderOptions(30-32)ReleaseIdentifier(1-5)packages/release-manager/src/db-variable-providers.ts (3)
DatabaseResourceVariableProvider(22-50)DatabaseDeploymentVariableProvider(68-126)DatabaseSystemVariableSetProvider(132-173)
packages/release-manager/src/repositories/release-repository.ts (1)
packages/release-manager/src/types.ts (3)
ReleaseRepository(34-38)ReleaseIdentifier(1-5)Release(14-17)
🪛 Biome (1.9.4)
packages/release-manager/src/repositories/release-repository.ts
[error] 9-9: This parameter is used before its declaration.
The parameter is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (14)
packages/release-manager/src/releases.ts (5)
1-2: No concerns regarding the lodash import
The import is properly utilized for_.isEqualin the subsequent code. Nothing else needs changing.
8-18: Interface definition looks clean and consistent
TheReleaseCreatorinterface provides clear expectations for the release creation workflow. No improvements needed.
35-36: Good filtering approach fornullvariables
Using this type guard ensuresvariablesexcludes allnullentries while preserving type safety in the subsequent logic.
53-63: Verify you're ignoringidandsensitivefields intentionally
By converting thevariablesarray into a{ [key: string]: any }object keyed byvariable.key, the code only compares thekeyandvalueproperties. If you need to consider other properties (e.g.,id,sensitive), confirm their omission is desired.
65-68: Conditional logic for reusing the latest release
This condition avoids creating duplicate releases whenversionIdand variable mappings match. The approach is straightforward and efficient.packages/release-manager/src/repositories/release-repository.ts (4)
9-9: Check the usage of the defaultdbparameter
A static analysis warning indicates potential misuse of the parameter before declaration. Since the import is defined above, the code appears valid, but confirm if reordering or removing the default parameter is necessary to satisfy the lint rule.🧰 Tools
🪛 Biome (1.9.4)
[error] 9-9: This parameter is used before its declaration.
The parameter is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
11-25: Query logic for retrieving the latest release
The method correctly filters by resource, deployment, and environment, ordering by descending creation time and returning null if none found.
27-38: Release creation flow is consistent
Merging the newly inserted database fields with the originalreleasedata ensures a complete returned object.
40-61: Conflict resolution approach is straightforward
UsingonConflictDoUpdateto update the desired release ID works well for preventing duplicate entries and keeping the table consistent.packages/release-manager/src/providers/variable-provider-factories.ts (3)
13-17: Implementation looks good.The
ResourceVariableProviderFactoryimplementation correctly implements theVariableProviderFactoryinterface and instantiates the appropriate provider.
19-23: Implementation looks good.The
DeploymentVariableProviderFactoryimplementation correctly implements theVariableProviderFactoryinterface and instantiates the appropriate provider.
25-29: Implementation looks good.The
SystemVariableSetProviderFactoryimplementation correctly implements theVariableProviderFactoryinterface and instantiates the appropriate provider.packages/release-manager/src/db-variable-providers.ts (2)
22-50: DatabaseResourceVariableProvider implementation looks good.The implementation correctly handles loading and retrieving resource variables from the database. It uses good patterns like memoization for the variables promise.
90-125: Hardcodedsensitiveproperty and potential object spread issues.The method sets
sensitive: falsefor all variables, which might not be accurate if some variables should be considered sensitive. Additionally, using the spread operator withvalueandvariable.defaultValuecould potentially override explicitly set properties if they contain keys likeid,key, orsensitive.if (res != null) return { id: variable.id, key, - sensitive: false, + sensitive: variable.sensitive ?? false, - ...value, + value: value.value, + resourceSelector: value.resourceSelector, }; if (variable.defaultValue != null) return { id: variable.id, key, - sensitive: false, + sensitive: variable.sensitive ?? false, - ...variable.defaultValue, + value: variable.defaultValue.value, + resourceSelector: variable.defaultValue.resourceSelector, };
| async getVariable(key: string): Promise<MaybeVariable> { | ||
| const variables = await this.getVariables(); | ||
| const variable = variables.find((v) => v.key === key) ?? null; | ||
| if (variable == null) return null; | ||
| return { | ||
| id: variable.id, | ||
| key, | ||
| value: variable.value, | ||
| sensitive: false, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix hardcoded sensitive flag in SystemVariableSetProvider.
Similar to the issue in DatabaseDeploymentVariableProvider, the sensitive property is hardcoded to false. Consider preserving the sensitive flag from the original variable if available:
async getVariable(key: string): Promise<MaybeVariable> {
const variables = await this.getVariables();
const variable = variables.find((v) => v.key === key) ?? null;
if (variable == null) return null;
return {
id: variable.id,
key,
value: variable.value,
- sensitive: false,
+ sensitive: variable.sensitive ?? false,
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async getVariable(key: string): Promise<MaybeVariable> { | |
| const variables = await this.getVariables(); | |
| const variable = variables.find((v) => v.key === key) ?? null; | |
| if (variable == null) return null; | |
| return { | |
| id: variable.id, | |
| key, | |
| value: variable.value, | |
| sensitive: false, | |
| }; | |
| } | |
| async getVariable(key: string): Promise<MaybeVariable> { | |
| const variables = await this.getVariables(); | |
| const variable = variables.find((v) => v.key === key) ?? null; | |
| if (variable == null) return null; | |
| return { | |
| id: variable.id, | |
| key, | |
| value: variable.value, | |
| sensitive: variable.sensitive ?? false, | |
| }; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/release-manager/src/variables.ts (4)
18-28: Consider adding error handling for database query failuresThe
getDeploymentVariableKeysfunction doesn't have any error handling for potential database query failures. If the query fails, it might lead to unexpected behavior in the calling code.const getDeploymentVariableKeys = async (options: { deploymentId: string; db?: Tx; }): Promise<string[]> => { const tx = options.db ?? db; - return tx - .select({ key: schema.deploymentVariable.key }) - .from(schema.deploymentVariable) - .where(eq(schema.deploymentVariable.deploymentId, options.deploymentId)) - .then((results) => results.map((r) => r.key)); + try { + const results = await tx + .select({ key: schema.deploymentVariable.key }) + .from(schema.deploymentVariable) + .where(eq(schema.deploymentVariable.deploymentId, options.deploymentId)); + return results.map((r) => r.key); + } catch (error) { + console.error('Failed to get deployment variable keys:', error); + return []; + } };
35-44: Database transaction not explicitly handled in static methodThe
databasestatic method creates providers and gets keys using database operations but doesn't explicitly handle the database transaction. Consider wrapping the operations in a try-catch block or adding error handling.static async database(options: VariableProviderOptions) { - const providers = [ - new DatabaseSystemVariableSetProvider(options), - new DatabaseResourceVariableProvider(options), - new DatabaseDeploymentVariableProvider(options), - ]; - - const keys = await getDeploymentVariableKeys(options); - return new VariableManager({ ...options, keys }, providers); + try { + const providers = [ + new DatabaseSystemVariableSetProvider(options), + new DatabaseResourceVariableProvider(options), + new DatabaseDeploymentVariableProvider(options), + ]; + + const keys = await getDeploymentVariableKeys(options); + return new VariableManager({ ...options, keys }, providers); + } catch (error) { + console.error('Failed to initialize VariableManager from database:', error); + throw error; // Re-throw to allow caller to handle + } }
64-70: Consider adding documentation for fallback behavior in getVariableThe
getVariablemethod iterates through providers and returns the first found variable, falling back tonullif none is found. This behavior is not documented. Consider adding a JSDoc comment to explain this behavior to users of this method.+/** + * Gets a variable by key from the first provider that returns a non-null value. + * @param key - The key to look up. + * @returns The variable if found, null otherwise. + */ async getVariable(key: string): Promise<MaybeVariable> { for (const provider of this.variableProviders) { const variable = await provider.getVariable(key); if (variable) return variable; } return null; }
60-62: The getVariables method could be optimizedThe current implementation of
getVariablescreates multiple promises that sequentially check each provider. This might not be the most efficient approach if you have many keys and providers.For better performance, consider implementing a batched approach where you can get variables for all keys from each provider in one operation if your providers support batch operations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/release-manager/src/repositories/release-repository.ts(1 hunks)packages/release-manager/src/variables.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/release-manager/src/repositories/release-repository.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/release-manager/src/variables.ts
🧬 Code Definitions (1)
packages/release-manager/src/variables.ts (1)
packages/release-manager/src/types.ts (3)
VariableProviderOptions(30-32)VariableProvider(22-24)MaybeVariable(20-20)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (3)
packages/release-manager/src/variables.ts (3)
30-32: ReleaseManagerOptions type should be exportedThe
VariableManagerOptionstype is used by theVariableManagerclass but isn't exported, making it unavailable to consumers who might need to construct these options.-type VariableManagerOptions = VariableProviderOptions & { +export type VariableManagerOptions = VariableProviderOptions & { keys: string[]; };
51-53: LGTM: Good defensive copy in getProviders methodMaking a shallow copy of the providers array in the
getProvidersmethod is good practice as it prevents direct manipulation of the internal array.
55-58: LGTM: Good method chaining pattern in addProviderThe
addProvidermethod returningthisallows for method chaining, which provides a nice fluent interface for consumers.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/release-manager/src/releases.ts (1)
65-77: 🛠️ Refactor suggestionRevisit variable comparison logic
The current implementation only compares
versionIdand variable key/value pairs, ignoring other properties likesensitivethat might impact releases.const latestR = { versionId: latestRelease?.versionId, variables: Object.fromEntries( - latestRelease?.variables.map((v) => [v.key, v.value]) ?? [], + latestRelease?.variables.map((v) => [v.key, { + value: v.value, + sensitive: v.sensitive, + }]) ?? [], ), }; const newR = { versionId, variables: Object.fromEntries( - nonNullVariables.map((v) => [v.key, v.value]), + nonNullVariables.map((v) => [v.key, { + value: v.value, + sensitive: v.sensitive, + }]), ), };packages/release-manager/src/db-variable-providers.ts (2)
118-124: 🛠️ Refactor suggestionFix hardcoded
sensitiveproperty and potential object spread issuesThe method sets
sensitive: falsefor all variables, which might not be accurate if some variables should be considered sensitive. Additionally, using the spread operator withvaluecould potentially override explicitly set properties.if (res != null) return { id: variable.id, key, - sensitive: false, - ...value, + sensitive: value.sensitive ?? false, + value: value.value, + resourceSelector: value.resourceSelector, };
180-185: 🛠️ Refactor suggestionFix hardcoded
sensitiveflag in SystemVariableSetProviderThe
sensitiveproperty is hardcoded tofalse, which might not correctly reflect the actual sensitivity of the variable.return { id: variable.id, key, value: variable.value, - sensitive: false, + sensitive: variable.sensitive ?? false, };
🧹 Nitpick comments (8)
packages/release-manager/src/releases.ts (3)
11-11: Consider exporting the ReleaseWithId typeThis type is used in the ReleaseCreator interface but isn't exported, which might limit reusability.
-type ReleaseWithId = Release & { id: string }; +export type ReleaseWithId = Release & { id: string };
28-28: Initialize repository with dependency injection patternThe current implementation directly instantiates the DatabaseReleaseRepository, making the class tightly coupled and harder to test.
- protected repository: ReleaseRepository = new DatabaseReleaseRepository(); + protected repository: ReleaseRepository; + + constructor( + protected options: ReleaseIdentifier, + repository?: ReleaseRepository + ) { + this.repository = repository ?? new DatabaseReleaseRepository(); + }This would require removing the existing constructor and updating call sites.
78-82: Consider optimizing the comparison logicThe current implementation retrieves the latest release and compares all variables even when the version IDs are different. A quick version ID check first could improve performance.
- return latestRelease != null && _.isEqual(latestR, newR) - ? latestRelease - : this.createRelease(versionId, nonNullVariables); + // Early return if versions differ or no latest release exists + if (latestRelease == null || latestRelease.versionId !== versionId) { + return this.createRelease(versionId, nonNullVariables); + } + + // Only compare variables if versions match + return _.isEqual(latestR.variables, newR.variables) + ? latestRelease + : this.createRelease(versionId, nonNullVariables);packages/release-manager/src/db-variable-providers.ts (1)
67-77: Consider adding more specific types for resource selectorsThe
resourceSelectorproperty is typed asanyin theDeploymentVariableValuetype, which could lead to type safety issues.type DeploymentVariableValue = { value: any; - resourceSelector: any; + resourceSelector: Record<string, string | number | boolean>; };packages/release-manager/src/types.ts (4)
7-12: Consider adding constraints for Variable typeThe
Variabletype is well-defined but lacks constraints or documentation about expected values, especially for thesensitiveflag.+/** + * Represents a variable with a value and metadata. + * @template T The type of the variable value + */ export type Variable<T = any> = { id: string; key: string; value: T; + /** Indicates whether the variable contains sensitive information and should be handled securely */ sensitive: boolean; };
30-32: Avoid usinganyfor db type in VariableProviderOptionsUsing
anyfor the db type reduces type safety and can lead to runtime errors.export type VariableProviderOptions = ReleaseIdentifier & { - db?: any; + db?: Tx; // Assuming Tx is the correct type from @ctrlplane/db };
34-42: Consider adding return type for setDesiredReleaseThe
setDesiredReleasemethod returnsPromise<any>, which is not type-safe. Consider defining a specific return type.setDesiredRelease( options: ReleaseIdentifier & { desiredReleaseId: string }, - ): Promise<any>; + ): Promise<{ id: string } | null>;
44-44: Consider removing redundant ReleaseQueryOptions typeThis type is just an alias for
ReleaseIdentifierand doesn't add any semantic meaning or additional properties.-export type ReleaseQueryOptions = ReleaseIdentifier;Then update any references to use
ReleaseIdentifierdirectly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/release-manager/README.md(1 hunks)packages/release-manager/src/db-variable-providers.ts(1 hunks)packages/release-manager/src/index.ts(1 hunks)packages/release-manager/src/manager.ts(1 hunks)packages/release-manager/src/releases.ts(1 hunks)packages/release-manager/src/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/release-manager/src/index.ts
- packages/release-manager/src/manager.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/release-manager/src/releases.tspackages/release-manager/src/db-variable-providers.tspackages/release-manager/src/types.ts
🧬 Code Definitions (2)
packages/release-manager/src/releases.ts (2)
packages/release-manager/src/types.ts (4)
Release(14-17)MaybeVariable(20-20)ReleaseIdentifier(1-5)ReleaseRepository(34-42)packages/release-manager/src/repositories/release-repository.ts (1)
DatabaseReleaseRepository(20-76)
packages/release-manager/src/db-variable-providers.ts (1)
packages/release-manager/src/types.ts (4)
ReleaseIdentifier(1-5)VariableProvider(22-24)Variable(7-12)MaybeVariable(20-20)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (7)
packages/release-manager/README.md (5)
20-21: LGTM! Clear installation command.The installation command correctly uses
pnpmand specifies the workspace dependency, which is a good practice for monorepo setups.
27-33: Well-documented variable resolution hierarchyThe clear explanation of the variable types and their priority hierarchy will help developers understand how variables are resolved in different contexts.
45-51: Good example setup for the release managerThe example demonstrates how to properly initialize the release manager with in-memory storage and a UUID generator, which is helpful for getting started.
167-192: Comprehensive rule engine exampleThe rule engine example effectively demonstrates how to create complex conditions by combining
ContextSpecificConditionandSemverCondition, and how to attach an action to a rule.
197-210: Nice database integration exampleThe example clearly shows how to switch from in-memory to database storage, which is important for production use.
packages/release-manager/src/db-variable-providers.ts (1)
38-48: Good variable loading implementationThe method properly loads and maps database variables while preserving the sensitive flag, which is important for security concerns.
packages/release-manager/src/types.ts (1)
1-5: Well-defined ReleaseIdentifier typeThe type clearly defines the required identifiers for a release, which is essential for the release management system.
| if (variable.defaultValue != null) | ||
| return { | ||
| id: variable.id, | ||
| key, | ||
| sensitive: false, | ||
| ...variable.defaultValue, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix hardcoded sensitive flag in default value handling
Similar to the previous issue, the sensitive property is hardcoded to false and using the spread operator could potentially override explicit properties.
if (variable.defaultValue != null)
return {
id: variable.id,
key,
- sensitive: false,
- ...variable.defaultValue,
+ sensitive: variable.defaultValue.sensitive ?? false,
+ value: variable.defaultValue.value,
+ resourceSelector: variable.defaultValue.resourceSelector,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (variable.defaultValue != null) | |
| return { | |
| id: variable.id, | |
| key, | |
| sensitive: false, | |
| ...variable.defaultValue, | |
| }; | |
| if (variable.defaultValue != null) | |
| return { | |
| id: variable.id, | |
| key, | |
| sensitive: variable.defaultValue.sensitive ?? false, | |
| value: variable.defaultValue.value, | |
| resourceSelector: variable.defaultValue.resourceSelector, | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/release-manager/src/release-new-verion.ts (3)
1-3: Check for potential underscore usage overhead.
Currently loading the entire_library when only a few methods might be used can introduce unnecessary overhead. Consider selectively importing only the necessary functions (e.g.,import { map, flatten } from "lodash";) to reduce bundle size and enhance clarity.
9-37: Consider streamlining the async mapping logic.
While using_.mapin conjunction withPromise.allworks, it might be clearer and more direct to use native JavaScript array functions (e.g.,await Promise.all(environments.map(...))) alongside a simple.flat()orArray.prototype.flatMapif available. Reducing external dependencies here can simplify the logic.-const resources = await _(environments) - .map(async (env) => { - ... - }) - .thru((promises) => Promise.all(promises)) - .value() - .then((arrays) => arrays.flat()); +const resources = (await Promise.all( + environments.map(async (env) => { + ... + return res.map((r) => ({ ...r, environment: env })); + }) +)).flat();
55-67: Evaluate partial failure handling.
If one of theensureReleasecalls fails, the entirePromise.allwill reject. This might leave partial updates in place. Consider whether you need a rollback mechanism or more granular error handling to avoid inconsistent release states.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/release-manager/src/index.ts(1 hunks)packages/release-manager/src/manager.ts(1 hunks)packages/release-manager/src/release-new-verion.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/release-manager/src/manager.ts
- packages/release-manager/src/index.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/release-manager/src/release-new-verion.ts
🧬 Code Definitions (1)
packages/release-manager/src/release-new-verion.ts (1)
packages/release-manager/src/manager.ts (1)
ReleaseManager(15-56)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/release-manager/src/releases.ts (1)
65-78: 🛠️ Refactor suggestionRevisit variable comparison logic
You're constructing objects using only{ key, value }pairs when generatinglatestRandnewR, ignoring other fields such asidandsensitive. If these fields are relevant to the release identity, the check in_.isEqual(latestR, newR)may return a false match or mismatch.Consider including all necessary properties or clarifying that only
keyandvaluematter for this comparison.
🧹 Nitpick comments (2)
packages/release-manager/src/releases.ts (2)
1-2: Consider reducing the bundle size by selectively importing from lodash
Importing the entire lodash library can increase bundle size if only a small subset of its utilities is required for this file.Here is an example approach if only
isEqualis needed:-import * as _ from "lodash"; +import isEqual from "lodash/isEqual";
39-54: Consider adding error handling or logging for repository failures
While a Promise rejection could be handled upstream, it might be beneficial to add logs or structured error responses here to aid debugging, especially since this code interacts with external resources (the database).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/release-manager/src/manager.ts(1 hunks)packages/release-manager/src/releases.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/release-manager/src/manager.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/release-manager/src/releases.ts
🧬 Code Definitions (1)
packages/release-manager/src/releases.ts (2)
packages/release-manager/src/types.ts (4)
Release(14-17)MaybeVariable(20-20)ReleaseIdentifier(1-5)ReleaseRepository(34-42)packages/release-manager/src/repositories/release-repository.ts (1)
DatabaseReleaseRepository(20-76)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
🔇 Additional comments (2)
packages/release-manager/src/releases.ts (2)
13-23: Interface structure appears consistent
TheReleaseCreatorinterface reads clearly and matches the usage in the implementing class. No discrepancies found between the interface and its implementation.
79-85: Release creation logic is straightforward and clear
If the release matches exactly, reuse the latest release; otherwise, create a new one. This approach is simple and efficient.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
packages/release-manager/src/repositories/types.ts (1)
6-8: Consider using a more specific return type thanPromise<any>Using
anyas a return type reduces type safety and makes it harder for consumers to understand what to expect from this method.setDesiredRelease( options: ReleaseIdentifier & { desiredReleaseId: string }, - ): Promise<any>; + ): Promise<{ environmentId: string; deploymentId: string; resourceId: string; desiredReleaseId: string } | null>;packages/release-manager/src/variables/types.ts (2)
1-1: Use relative imports for consistencyThe import statement is using an absolute path that starts with "src/", which can cause issues depending on how the project is structured. Consider using a relative import path for consistency with other files.
-import type { ReleaseIdentifier } from "src/types"; +import type { ReleaseIdentifier } from "../types.js";
21-23: Avoid usinganytype for better type safetyThe
dbproperty is typed asanywhich reduces type safety. Consider using a more specific type that represents the database connection or transaction.export type VariableProviderOptions = ReleaseIdentifier & { - db?: any; + db?: Tx; // Assuming Tx is the transaction type from @ctrlplane/db };packages/release-manager/src/manager.ts (1)
40-48: Consider handling potential errors in theensureReleasemethodThe method awaits async operations without error handling. While the coding guidelines allow for this pattern, consider adding error handling for better robustness.
async ensureRelease(versionId: string, opts?: { setAsDesired?: boolean }) { - const variables = await this.getCurrentVariables(); - const { created, release } = await this.releaseCreator.ensureRelease( - versionId, - variables, - ); - if (opts?.setAsDesired) await this.setDesiredRelease(release.id); - return { created, release }; + try { + const variables = await this.getCurrentVariables(); + const { created, release } = await this.releaseCreator.ensureRelease( + versionId, + variables, + ); + if (opts?.setAsDesired) await this.setDesiredRelease(release.id); + return { created, release }; + } catch (error) { + // Log the error and rethrow or handle appropriately + console.error('Error ensuring release:', error); + throw error; + } }packages/release-manager/src/releases.ts (1)
8-8: Unify duplicatedReleaseWithIdtype definition.A similar type (
ReleaseWithId) is also defined inpackages/release-manager/src/types.ts. Consider centralizing its definition and importing it here to avoid duplication or inconsistencies.- type ReleaseWithId = Release & { id: string }; + import type { ReleaseWithId } from "../types.js";packages/release-manager/src/variables/variables.ts (1)
64-68: Clarify the priority among providers.The loop returns the first non-null variable from providers in this order: system, resource, deployment. If there's a more logical order or if merging data from multiple providers is desired, consider adjusting the process or documenting the priority clearly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
packages/release-manager/src/index.ts(1 hunks)packages/release-manager/src/manager.ts(1 hunks)packages/release-manager/src/releases.ts(1 hunks)packages/release-manager/src/repositories/types.ts(1 hunks)packages/release-manager/src/types.ts(1 hunks)packages/release-manager/src/variables/db-variable-providers.ts(1 hunks)packages/release-manager/src/variables/types.ts(1 hunks)packages/release-manager/src/variables/variables.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/release-manager/src/types.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/release-manager/src/index.tspackages/release-manager/src/manager.tspackages/release-manager/src/repositories/types.tspackages/release-manager/src/variables/db-variable-providers.tspackages/release-manager/src/releases.tspackages/release-manager/src/variables/types.tspackages/release-manager/src/variables/variables.ts
🧬 Code Definitions (6)
packages/release-manager/src/manager.ts (4)
packages/release-manager/src/types.ts (1)
ReleaseIdentifier(3-7)packages/release-manager/src/releases.ts (1)
BaseReleaseCreator(22-83)packages/release-manager/src/variables/variables.ts (1)
VariableManager(34-71)packages/release-manager/src/repositories/release-repository.ts (1)
DatabaseReleaseRepository(20-76)
packages/release-manager/src/repositories/types.ts (1)
packages/release-manager/src/types.ts (3)
ReleaseIdentifier(3-7)ReleaseWithId(14-14)Release(9-12)
packages/release-manager/src/variables/db-variable-providers.ts (2)
packages/release-manager/src/types.ts (1)
ReleaseIdentifier(3-7)packages/release-manager/src/variables/types.ts (3)
VariableProvider(13-15)Variable(3-8)MaybeVariable(11-11)
packages/release-manager/src/releases.ts (4)
packages/release-manager/src/types.ts (3)
ReleaseWithId(14-14)Release(9-12)ReleaseIdentifier(3-7)packages/release-manager/src/variables/types.ts (1)
MaybeVariable(11-11)packages/release-manager/src/repositories/types.ts (1)
ReleaseRepository(3-9)packages/release-manager/src/repositories/release-repository.ts (1)
DatabaseReleaseRepository(20-76)
packages/release-manager/src/variables/types.ts (1)
packages/release-manager/src/types.ts (1)
ReleaseIdentifier(3-7)
packages/release-manager/src/variables/variables.ts (2)
packages/release-manager/src/variables/types.ts (3)
VariableProviderOptions(21-23)VariableProvider(13-15)MaybeVariable(11-11)packages/release-manager/src/variables/db-variable-providers.ts (3)
DatabaseSystemVariableSetProvider(142-183)DatabaseResourceVariableProvider(26-54)DatabaseDeploymentVariableProvider(75-133)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (6)
packages/release-manager/src/index.ts (1)
1-6: Well-structured index exportsThis index file provides a clean central point for package exports, making it easier for consumers to import from the package.
packages/release-manager/src/manager.ts (3)
22-28: Well-structured constructor with dependency injectionThe constructor properly initializes dependencies and allows for database transaction injection, which is good for testing and flexibility.
30-38: Clean implementation of lazy initializationThe
getCurrentVariablesmethod efficiently implements lazy initialization of the variable manager, which is a good pattern for reducing unnecessary object creation.
50-55: Clean implementation ofsetDesiredReleaseThis method efficiently delegates to the repository layer and passes along the necessary parameters.
packages/release-manager/src/releases.ts (1)
80-80: Ensure error handling for repository call.
this.createReleasemay fail due to database or networking issues. Consider ensuring that any thrown errors are handled or propagated properly, especially in production scenarios.packages/release-manager/src/variables/variables.ts (1)
37-40: Add or verify test coverage for multiple providers.Multiple providers are created (system, resource, deployment). Ensure that test cases include scenarios where two or more providers define a variable with the same key so that the correct resolution order is tested.
| sensitive: false, | ||
| ...value, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve sensitivity for deployment variable values and default values.
Similar to system variables, these lines unconditionally set sensitive to false. If the deployment logic relies on the sensitive status, consider incorporating a corresponding field to correctly reflect whether the variable is sensitive in the database model.
- sensitive: false,
+ sensitive: Boolean(value.sensitive),(or add a dedicated field in DeploymentVariableValue if needed)
Also applies to: 127-128
| id: variable.id, | ||
| key, | ||
| value: variable.value, | ||
| sensitive: false, | ||
| }; |
There was a problem hiding this comment.
Review handling of sensitive field.
This provider explicitly sets sensitive to false instead of using the underlying database value. If some system variables are intended to be flagged as sensitive, this code masks that distinction. Please confirm whether ignoring a stored sensitive status could lead to accidental exposure of sensitive data.
- sensitive: false,
+ sensitive: variable.sensitive,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| id: variable.id, | |
| key, | |
| value: variable.value, | |
| sensitive: false, | |
| }; | |
| id: variable.id, | |
| key, | |
| value: variable.value, | |
| sensitive: variable.sensitive, | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/release-manager/README.md (5)
20-34: ReleaseManager Code ExampleThe TypeScript code snippet demonstrates how to instantiate a ReleaseManager using its static factory method, upsert a release, and set the desired release. The example is well-structured and realistic.
Suggestion: Consider adding a brief comment or error handling note for production usage to signal what steps to take if, for example, the asynchronous operations fail.
51-74: Repository Layer Code ExamplesThe provided snippets illustrating repository interactions—both for direct creation and upsert operations—demonstrate a clean and consistent API.
Suggestion: It might be worthwhile to include a brief note or comment on error handling in these examples to remind users of best practices when integrating with a real database.
76-91: How It Works Section & Wording RefinementsThe "How It Works" section outlines the complete release flow effectively:
- The description under bullet point 2 uses the phrase "exact same version and variables exists".
Suggestion: Consider simplifying this to "same version and variables" to improve readability.- Additionally, check for consistent punctuation in the bullet points (e.g., adding a period at the end of the statement on line 85).
🧰 Tools
🪛 LanguageTool
[style] ~81-~81: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ...The system checks if a release with the exact same version and variables already exists: ...(EN_WORDINESS_PREMIUM_EXACT_SAME)
[uncategorized] ~85-~85: A period might be missing here.
Context: ... creates a new release with the current variables 3. Version Release Flow: When a ne...(AI_EN_LECTOR_MISSING_PUNCTUATION_PERIOD)
102-123: Integration Example UsageThe integration example demonstrates how to use the
ReleaseManagerin a real-world scenario. It effectively covers the instantiation, release creation (with thesetAsDesiredoption), and logging of outcomes.
Suggestion: Optionally, consider mentioning the expected behavior of the log outputs to guide users on verifying correct integration.
125-133: Release New Version Flow ExampleThe snippet covering the new version release flow is succinct and illustrative. Calling
releaseNewVersionshows how to create releases for all resources matching a deployment version.
Suggestion: You might add a comment to explain any critical error handling strategies or failure conditions that users should be aware of.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/release-manager/README.md(1 hunks)packages/release-manager/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/release-manager/package.json
🧰 Additional context used
🪛 LanguageTool
packages/release-manager/README.md
[style] ~81-~81: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ...The system checks if a release with the exact same version and variables already exists: ...
(EN_WORDINESS_PREMIUM_EXACT_SAME)
[uncategorized] ~85-~85: A period might be missing here.
Context: ... creates a new release with the current variables 3. Version Release Flow: When a ne...
(AI_EN_LECTOR_MISSING_PUNCTUATION_PERIOD)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint
🔇 Additional comments (7)
packages/release-manager/README.md (7)
1-5: Documentation Header and Introductory TextThe title and introductory lines set a clear context for the package. The description ("A streamlined solution for managing context-specific software releases…") is concise and engaging.
6-12: Overview Section ClarityThe overview explains the package functionality well, describing its role in handling variable resolution and release creation. The language is accessible and informative without overcomplicating the details.
13-19: Key Components IntroductionThe "Key Components" section clearly introduces the primary entity, ReleaseManager, and its responsibilities. The explanation is concise and aids readers in understanding the high-level architecture.
36-45: Variable System ExplanationThe explanation of the variable resolution order (resource, deployment, and system variables) is clear and informative. Presenting the priorities in a numbered list supports quick comprehension.
46-49: Current Variables Retrieval ExampleThe code snippet showing the retrieval of current variables via
manager.getCurrentVariables()is straightforward and useful for users looking for a quick reference.
92-100: Testing InstructionsThe testing section provides clear commands for running tests and type checks. The instructions are concise and actionable.
135-138: License and Parent Repository InformationThe license section and parent repository link are correctly provided. This offers clear guidance on project governance and source reference.
Summary by CodeRabbit
New Features
ReleaseManagerinterface for coordinating variable retrieval and release creation.ReleaseNewVersionevents, enhancing the release process.Documentation
README.mdfile outlining the framework for managing software releases.