init deny rule #421
init deny rule #421
Conversation
WalkthroughThis pull request introduces several new files and functionality in the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant RE as RuleEngine
participant RD as DeploymentDenyRule
C->>RE: Call evaluate(releases, context)
RE->>RE: Iterate through each rule
RE->>RD: Invoke filter(context, releases)
RD-->>RE: Return filtered releases or denial result
RE->>RE: Check if any rule denies deployment
RE->>RE: Call selectFinalRelease(context, candidates)
RE-->>C: Return final deployment selection result
Poem
✨ 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: 0
🧹 Nitpick comments (11)
packages/rule-engine/README.md (1)
1-2: README is a good starting point.
The title "Rule Engine" establishes the package identity; however, expanding documentation with usage examples, setup instructions, and further details would be beneficial for onboarding new developers.packages/rule-engine/src/rules/index.ts (1)
1-2: Export statement review.
The export of"./deployment-deny-rule.js"provides a clear consolidation of rule-related exports. Please verify that the file extension aligns with your build output. If the source file is written in TypeScript (i.e..ts), you might consider using an extension-less import (e.g../deployment-deny-rule) to maintain consistency with TypeScript’s module resolution.packages/rule-engine/src/rules/__tests__/deployment-deny-rule.test.ts (1)
185-273: Consider re-enabling or refining the DST tests.The commented-out DST tests provide valuable coverage for edge cases around clock changes. If they're incomplete or flaky, consider stabilizing them or removing them to avoid confusion. If you need help finalizing this DST logic, let me know!
packages/rule-engine/src/rules/deployment-deny-rule.ts (2)
138-199: Remove or replace verbose console logs with a proper logger.Multiple
console.logcalls here are helpful for debugging but may create noise in production. Consider using a configurable logger or removing these statements for cleaner output.- console.log("o", o); - console.log("------"); - console.log("timezone", this.timezone); - ... + // Use a logger with log levels or remove entirely
265-274: Remove or utilize the unused helper function.The
getTimeOfDayInSecondsfunction is currently not used. Either remove it to keep the code tidy or incorporate it into the logic if intended for future use.- private getTimeOfDayInSeconds(date: Date): number { - return date.getHours() * 3600 + date.getMinutes() * 60 + date.getSeconds(); - }packages/rule-engine/src/rule-engine.ts (3)
52-65: Consider simplifying the rules type.The current constructor signature allows each item in the
rulesarray to be either a function that returns aDeploymentResourceRuleor aDeploymentResourceRuleitself. While this approach is flexible, it can introduce complexity when reading or maintaining the code. One possibility is to standardize on a single approach—either always passing in rules as callable factories or always passing in instantiated rule objects. This would reduce type complexity and make the intent clearer.
94-126: Consider logging partial rule outcomes for debugging.The
evaluatemethod properly short-circuits on a rule that disqualifies all candidate releases. However, if multiple rules participate in filtering, it can be challenging to see each rule’s impact on the release set. Adding optional logging or collected results after each rule can help in debugging or auditing deployment decisions. This extra transparency can be quite valuable in complex rule chains.
149-164: Offer a pluggable final selection strategy.The
selectFinalReleasemethod implements a clear selection strategy based on sequential upgrades, a desired release ID, or the newest release. For advanced use cases where a different priority might be needed (e.g., stability vs. latest features), consider making this selection strategy pluggable or configurable. This would allow consumers of the RuleEngine to tailor the final selection logic without modifying the core engine.packages/rule-engine/src/releases.ts (2)
3-17: Update documentation references from "CandidateReleases" to match the actual class name.The doc comments refer to "CandidateReleases" while the class is named
Releases. This could be confusing for developers reading the documentation. Consider renaming these references in the docstrings for consistency:- * A class that encapsulates candidate releases with utility methods for common - * operations. This class is used throughout the rule engine to provide consistent handling - * of release collections. Rules should operate on CandidateReleases instances... + * A class that encapsulates release collections with utility methods for common + * operations. This class is used throughout the rule engine to provide consistent handling + * of release collections. Rules should operate on Releases instances...
22-31: Consider immutability for thereleasesarray.Currently, the
Releasesclass copies the array in its constructor, which is a good practice. You could further enhance immutability by marking the private array as read-only (e.g.,private readonly releases: Release[]) or returning defensive copies more systematically. This helps prevent accidental modifications to the internal array state, promoting safer functional operations.packages/rule-engine/src/types.ts (1)
3-13: Consider strongly typing theconfigproperty.The
configfield in theversionobject usesRecord<string, any>, which can be too broad and may lead to unchecked usage. If your application deals with a well-known configuration schema, you could introduce a generic or partial type forconfigto provide stronger compile-time validation and offer clearer development-time assistance.
📜 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 (12)
packages/rule-engine/README.md(1 hunks)packages/rule-engine/eslint.config.js(1 hunks)packages/rule-engine/package.json(1 hunks)packages/rule-engine/src/index.ts(1 hunks)packages/rule-engine/src/releases.ts(1 hunks)packages/rule-engine/src/rule-engine.ts(1 hunks)packages/rule-engine/src/rules/__tests__/deployment-deny-rule.test.ts(1 hunks)packages/rule-engine/src/rules/deployment-deny-rule.ts(1 hunks)packages/rule-engine/src/rules/index.ts(1 hunks)packages/rule-engine/src/types.ts(1 hunks)packages/rule-engine/tsconfig.json(1 hunks)packages/rule-engine/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/rule-engine/src/rules/index.tspackages/rule-engine/src/index.tspackages/rule-engine/src/rules/deployment-deny-rule.tspackages/rule-engine/src/rules/__tests__/deployment-deny-rule.test.tspackages/rule-engine/vitest.config.tspackages/rule-engine/src/rule-engine.tspackages/rule-engine/src/releases.tspackages/rule-engine/src/types.ts
🧬 Code Definitions (3)
packages/rule-engine/src/rules/deployment-deny-rule.ts (2)
packages/rule-engine/src/types.ts (3)
DeploymentResourceRule(54-60)DeploymentResourceContext(33-38)DeploymentResourceRuleResult(40-43)packages/rule-engine/src/releases.ts (1)
Releases(18-302)
packages/rule-engine/src/rules/__tests__/deployment-deny-rule.test.ts (3)
packages/rule-engine/src/releases.ts (1)
Releases(18-302)packages/rule-engine/src/types.ts (2)
DeploymentResourceContext(33-38)Release(3-13)packages/rule-engine/src/rules/deployment-deny-rule.ts (1)
DeploymentDenyRule(53-274)
packages/rule-engine/src/releases.ts (1)
packages/rule-engine/src/types.ts (2)
Release(3-13)DeploymentResourceContext(33-38)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (7)
packages/rule-engine/tsconfig.json (1)
1-12: TSConfig is well-configured.
The file correctly extends the internal base configuration and specifies key compiler options such as the output directory, base URL, incremental compilation, and a custom tsBuildInfoFile path. One suggestion is to consider a more granular include pattern (e.g.,"src/**/*.ts") if nested TypeScript files are expected in the future.packages/rule-engine/vitest.config.ts (1)
1-15: Vitest configuration appears robust.
This configuration correctly sets up the Node.js test environment, includes tests from the designated folder, excludes the appropriate directories, and establishes a useful alias for the source folder. Everything looks in order for a modern testing setup.packages/rule-engine/src/index.ts (1)
1-5: Consolidated exports are clean.
The file provides a well-organized entry point by re-exporting modules from multiple sources. Just ensure that the use of the.jsextension is in line with your compiled output conventions when using TypeScript.packages/rule-engine/eslint.config.js (1)
1-13: Looks good overall.Using an array to merge multiple configs is a valid approach in ESLint. Just confirm that
"ignores": ["dist/**"]is recognized as expected in your setup, as some ESLint configurations may prefer a top-levelignorePatternsfield or a dedicated.eslintignorefile. Otherwise, no issues.packages/rule-engine/package.json (1)
1-41:❓ Verification inconclusive
Verify the catalog references.
Several dependencies specify
"catalog:"versions (e.g.,"zod": "catalog:"). If these references rely on an internal registry or a custom resolution mechanism, please ensure they are valid and available in your environment.Please see if these references resolve without errors during installation.
🏁 Script executed:
#!/bin/bash # Check for lines referencing "catalog:" in dependencies rg 'catalog:' package.json -A 3Length of output: 114
Catalog References Verification Needed:
The output indicates that at least one dependency (e.g.,"prettier": "catalog:") still relies on acatalog:reference, while others (like"typescript") now use a concrete version. Please confirm that your internal registry or custom resolution mechanism properly resolves allcatalog:references during installation. Verify that running your install command (e.g.,npm install) succeeds without errors and that all dependencies are correctly fetched.
- Location:
packages/rule-engine/package.json(notably the"prettier"dependency).- Action: Manually test the installation process in your environment to ensure that the internal registry handles these
catalog:entries without issues.packages/rule-engine/src/rules/__tests__/deployment-deny-rule.test.ts (1)
1-184: Tests look thorough and well-organized.You cover denial intervals, custom reasons, and time zones broadly. The usage of mocking for current time is appropriate and ensures reproducible results. Overall, this test suite offers good coverage of the rule’s functionality.
packages/rule-engine/src/types.ts (1)
55-60: No concurrency or performance concerns detected.The
DeploymentResourceRuleinterface is straightforward and relies on synchronous/asynchronous filtering logic. There are no evident concurrency or performance issues at this level. Overall, the design is flexible and should scale well for typical usage.
Co-authored-by: Justin Brooks <justin@wandb.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/rule-engine/src/rules/deployment-deny-rule.ts (3)
43-51: Add documentation for thedtendproperty.The
denyReasonproperty has good documentation, but thedtendproperty is missing JSDoc comments to explain its purpose and usage. Consider adding documentation to improve clarity.export interface DeploymentDenyRuleOptions extends Partial<RRuleOptions> { + /** + * Optional end date for the denied time period. + * When provided with dtstart, it defines the duration of each denial period. + */ dtend?: Date | null; /** * Custom reason to return when deployment is denied Defaults to "Deployment * denied due to time-based restrictions" */ denyReason?: string; }
121-158: Consider handling the case where only one of dtstart or dtend is provided.The current implementation requires both
dtstartanddtendto be non-null to use the interval check. Consider how to handle cases where only one date is provided or add validation to ensure this requirement is clear.private isDeniedTime(now: Date): boolean { // now is in whatever timezone of the server. We need to convert it to match // the timezone for the library const parts = getDatePartsInTimeZone(now, this.timezone); const nowDt = datetime( parts.year, parts.month, parts.day, parts.hour, parts.minute, parts.second, ); const occurrence = this.rrule.before(nowDt, true); // If there's no occurrence on or before the time, it's not in a denied // period if (occurrence == null) return false; // If dtend is specified, check if time is between occurrence and occurrence // + duration - if (this.dtend != null && this.dtstart != null) { + if (this.dtend != null && this.dtstart != null) { const dtstart = this.castTimezone(this.dtstart, this.timezone); const dtend = this.castTimezone(this.dtend, this.timezone); // Calculate duration in local time to handle DST correctly const durationMs = differenceInMilliseconds(dtend, dtstart); const occurrenceEnd = addMilliseconds(occurrence, durationMs); return isWithinInterval(nowDt, { start: occurrence, end: occurrenceEnd, }); + } else if (this.dtend != null && this.dtstart == null) { + // Handle case where only dtend is provided + // This could involve setting a default duration or using a different approach + // depending on the intended behavior + // Example: Use some default start time or the occurrence itself + const defaultStart = occurrence; + const dtend = this.castTimezone(this.dtend, this.timezone); + + return isWithinInterval(nowDt, { + start: defaultStart, + end: dtend, + }); } // If no dtend, check if the occurrence is on the same day using date-fns return isSameDay(occurrence, now, { in: tz(this.timezone) }); }
31-31: Consider a more robust default value handling.The current fallback to
"0"for missing date parts could lead to unexpected behavior if a date component is genuinely missing. Consider adding additional validation or using a more specific default.- const get = (type: string) => - parts.find((p) => p.type === type)?.value ?? "0"; + const get = (type: string) => { + const value = parts.find((p) => p.type === type)?.value; + if (value === undefined) { + console.warn(`Missing date part: ${type}`); + return "0"; + } + return value; + };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/rule-engine/src/rules/__tests__/deployment-deny-rule.test.ts(1 hunks)packages/rule-engine/src/rules/deployment-deny-rule.ts(1 hunks)packages/rule-engine/vitest.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rule-engine/vitest.config.ts
- packages/rule-engine/src/rules/tests/deployment-deny-rule.test.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/rule-engine/src/rules/deployment-deny-rule.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (4)
packages/rule-engine/src/rules/deployment-deny-rule.ts (4)
18-41: Well-structured timezone handling utility function.The
getDatePartsInTimeZonefunction leverages the nativeIntl.DateTimeFormatAPI for timezone conversions, which is the correct approach. The function cleanly parses date components from the formatted parts.
87-90: Good testing design with protected method.Making
getCurrentTimeprotected allows for easier testing by overriding it in a subclass. This is a good design pattern for time-dependent functionality.
92-113: Clean implementation of the filter method.The method has a clear logical flow and appropriate handling of the denied time case. Good use of helper methods to keep the code readable.
160-169: Simple and effective timezone conversion method.The
castTimezonemethod is well-documented and properly uses the TZDate from @date-fns/tz library for timezone conversions.
| export class DeploymentDenyRule implements DeploymentResourceRule { | ||
| public readonly name = "DeploymentDenyRule"; | ||
| private rrule: RRule; | ||
| private denyReason: string; | ||
| private dtend: Date | null; | ||
| private timezone: string; | ||
| private dtstart: Date | null; | ||
|
|
||
| constructor({ | ||
| denyReason = "Deployment denied due to time-based restrictions", | ||
| dtend = null, | ||
| dtstart = null, | ||
| until = null, | ||
| ...options | ||
| }: DeploymentDenyRuleOptions) { | ||
| this.timezone = options.tzid ?? "UTC"; | ||
| this.denyReason = denyReason; | ||
|
|
||
| const dtStartCasted = | ||
| dtstart != null ? this.castTimezone(dtstart, this.timezone) : null; | ||
|
|
||
| const untilCasted = | ||
| until != null ? this.castTimezone(until, this.timezone) : null; | ||
|
|
||
| this.rrule = new RRule({ | ||
| ...options, | ||
| tzid: "UTC", | ||
| dtstart: dtStartCasted, | ||
| until: untilCasted, | ||
| }); | ||
| this.dtstart = dtstart; | ||
| this.dtend = dtend; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider validating the timezone string.
The constructor correctly sets default values and handles timezone configuration, but there's no validation for the timezone string. An invalid timezone could lead to unexpected behavior.
Verify whether additional validation for timezone strings would be beneficial. The timezone is used in several places including castTimezone and date formatting, so invalid values could cause runtime errors.
🏁 Script executed:
#!/bin/bash
# Check if the code has any existing timezone validation elsewhere
rg -A 2 -B 2 "tzid|timezone|timeZone" --type ts packages/rule-engine/Length of output: 8352
Improve Timezone String Validation
- The constructor sets
this.timezoneusingoptions.tzid ?? "UTC", but there is no explicit validation to ensure that the provided string is a valid IANA timezone. - The timezone is later used in the
castTimezonemethod (which creates a newTZDate) and withIntl.DateTimeFormat, so an invalid timezone could indeed result in runtime errors. - It is recommended to add validation—either directly in the constructor or within
castTimezone—to verify that the timezone string conforms to the expected format. Integrating an existing timezone validation library or a regex check could be beneficial.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/rule-engine/README.md (1)
22-42: Good explanation with a minor grammar improvement opportunity.The explanation of how the rule engine works is thorough and easy to understand.
Consider adding "the" before "newest release" in line 36:
- Specified desired release if available - - Otherwise, newest release by creation date + - Otherwise, the newest release by creation date🧰 Tools
🪛 LanguageTool
[grammar] ~36-~36: A determiner may be missing.
Context: ...ed release if available - Otherwise, newest release by creation date 3. Tracking r...(THE_SUPERLATIVE)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/rule-engine/README.md(1 hunks)packages/rule-engine/src/rule-engine.ts(1 hunks)packages/rule-engine/src/rules/__tests__/deployment-deny-rule.test.ts(1 hunks)packages/rule-engine/src/rules/deployment-deny-rule.ts(1 hunks)packages/rule-engine/src/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rule-engine/src/rules/deployment-deny-rule.ts
- packages/rule-engine/src/rules/tests/deployment-deny-rule.test.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/rule-engine/src/rule-engine.tspackages/rule-engine/src/types.ts
🪛 LanguageTool
packages/rule-engine/README.md
[grammar] ~36-~36: A determiner may be missing.
Context: ...ed release if available - Otherwise, newest release by creation date 3. Tracking r...
(THE_SUPERLATIVE)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (14)
packages/rule-engine/README.md (4)
1-7: LGTM: Clear and concise introduction.The introduction effectively summarizes the purpose of the Rule Engine package.
8-20: LGTM: Well-structured overview of core components.The overview and core components section provides a clear explanation of the package's architecture and main classes.
44-74: LGTM: Helpful usage example.The code example effectively demonstrates how to use the Rule Engine, create rules, and handle results.
76-109: LGTM: Clear extension guidance.The extension documentation provides a good template for implementing custom rules.
packages/rule-engine/src/rule-engine.ts (5)
1-51: LGTM: Well-documented class with clear examples.The JSDoc comments provide excellent documentation for the RuleEngine class, including a comprehensive usage example.
52-65: LGTM: Clear constructor with flexible rule definition.The constructor allows for both direct rule instances and rule factories that return promises, providing good flexibility.
67-138: LGTM: Well-implemented evaluation logic.The evaluate method correctly:
- Processes rules sequentially
- Tracks rejection reasons across all rules
- Handles early termination when all candidates are disqualified
- Returns appropriate result structures
The implementation also includes thorough documentation for rule authors.
140-176: LGTM: Clear selection strategy implementation.The selectFinalRelease method implements the prioritization logic as described in the documentation:
- Sequential upgrades (oldest first)
- Desired release if specified
- Default to newest release
The implementation is concise and follows the single responsibility principle.
178-190: LGTM: Simple and focused helper method.The findSequentialUpgradeReleases method has a clear purpose and leverages the Releases utility methods appropriately.
packages/rule-engine/src/types.ts (5)
1-13: LGTM: Well-defined Release type.The Release type encompasses all necessary properties for a software release and uses appropriate type definitions.
15-31: LGTM: Clear entity type definitions.The Deployment, Resource, and Environment types are appropriately defined with necessary properties.
33-38: LGTM: Comprehensive context type.The DeploymentResourceContext type correctly combines all entities needed for rule evaluation.
40-49: LGTM: Well-defined result types.Both result types (DeploymentResourceRuleResult and DeploymentResourceSelectionResult) clearly express their purpose and contain all necessary information.
51-60: LGTM: Clear rule interface.The DeploymentResourceRule interface provides a simple but effective contract for implementing rules, with the appropriate flexibility to return either a direct result or a promise.
Summary by CodeRabbit