-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Remove semver and regex release filter #160
Conversation
WalkthroughThe pull request introduces significant changes across multiple components and files, primarily focusing on the handling of release conditions and schema validation. Key modifications include the integration of a new Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (9)
packages/db/drizzle/0019_stale_captain_marvel.sql (3)
1-1
: LGTM! Consider adding documentation for the new column.The addition of the
release_filter
column asjsonb
withDEFAULT NULL
is a good choice. It allows for flexible storage of release filter data and is backwards compatible with existing rows.Consider adding a comment in the migration file or updating the schema documentation to explain the purpose and expected structure of the
release_filter
JSON data.
3-3
: Consider adding 'IF EXISTS' for consistency.The removal of the
evaluate
column is in line with the PR objectives. However, for consistency with the previous statement and to ensure the migration doesn't fail if the column has already been removed, consider adding theIF EXISTS
clause.Apply this diff to add the
IF EXISTS
clause:-ALTER TABLE "environment_policy" DROP COLUMN IF EXISTS "evaluate"; +ALTER TABLE "environment_policy" DROP COLUMN IF EXISTS "evaluate";
1-3
: Summary: Migration aligns with PR objectives, consider transaction wrapping.This migration successfully updates the
environment_policy
table to support the new release filter system by adding arelease_filter
column and removing the oldevaluate_with
andevaluate
columns. The changes are backwards compatible and align with the PR objectives.Consider wrapping these ALTER TABLE statements in a transaction to ensure atomicity of the migration. This can be achieved by adding
BEGIN;
at the beginning of the file andCOMMIT;
at the end. This ensures that either all changes are applied or none are, maintaining database consistency in case of any failures during the migration process.packages/job-dispatch/src/policies/release-string-check.ts (1)
45-62
: Approve changes with a minor suggestion for improvementThe changes effectively streamline the release validation process by introducing the
releaseFilter
and simplifying the database query. This aligns well with the modifications made in other parts of the codebase, as mentioned in the PR summary.The code is now more maintainable and easier to understand. However, to further improve readability, consider extracting the database query into a separate function.
Here's a suggested refactoring to improve code readability:
async function findMatchingRelease(db, releaseId, releaseFilter) { return db .select() .from(schema.release) .where( and( eq(schema.release.id, releaseId), schema.releaseMatchesCondition(db, releaseFilter), ), ) .then(takeFirstOrNull); } // In the main function: const release = await findMatchingRelease(db, rel.id, releaseFilter);This extraction would make the main function more concise and easier to follow.
packages/validators/src/releases/conditions/release-condition.ts (1)
52-54
: LGTM! Consider adding a JSDoc comment for clarity.The
isEmptyCondition
function is well-implemented and serves its purpose effectively. It will be useful for simplifying condition checking in other parts of the codebase, particularly in theDeploymentControl.tsx
component.Consider adding a JSDoc comment to improve clarity:
/** * Checks if a given ReleaseCondition is an empty comparison condition. * @param condition - The ReleaseCondition to check. * @returns True if the condition is a Comparison type with an empty conditions array, false otherwise. */ export const isEmptyCondition = (condition: ReleaseCondition): boolean => condition.type === ReleaseFilterType.Comparison && condition.conditions.length === 0;apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx (1)
81-83
: Approve API call update with a minor suggestion.The update from
api.release.blockedEnvironments.useQuery
toapi.release.blocked.useQuery
aligns with the PR objective to simplify release filtering. The functionality remains unchanged as the parameters and options are the same.Consider renaming the
blockedEnvByRelease
variable toblockedReleases
orblockedReleasesQuery
to better reflect its new, potentially more generic nature:- const blockedEnvByRelease = api.release.blocked.useQuery(releaseIds, { + const blockedReleases = api.release.blocked.useQuery(releaseIds, { enabled: releaseIds.length > 0, });apps/webservice/src/app/[workspaceSlug]/_components/environment-policy-drawer/DeploymentControl.tsx (1)
126-132
: Add a description to the 'Filter' field to improve user guidanceIncluding a
FormDescription
can help users understand how to use the 'Filter' field effectively.Apply this diff to add a description:
<FormItem> <FormLabel>Filter</FormLabel> + <FormDescription> + Define conditions to filter which releases are deployed to this environment. + </FormDescription> <FormControl> <ReleaseConditionRender condition={value ?? defaultCondition} onChange={onChange} /> </FormControl>apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowPolicyNode.tsx (1)
277-277
: Consider adding a loading indicatorWhen
hasFilterCheck
is true andisFilterCheckLoading
is true, the component returns an empty div. Consider adding a visual loading indicator to improve user experience during data fetching.packages/api/src/router/release.ts (1)
Authorization check performance concerns confirmed
The simulation with 1000 release IDs reveals that the current implementation may indeed face performance challenges with large inputs. The linear scaling of processing time (1 second per ID) could lead to significant delays for bulk operations.
Recommendations:
- Optimize the authorization check to handle multiple IDs more efficiently.
- Consider batch processing or parallel authorization checks for large sets of release IDs.
- Implement a maximum limit on the number of IDs that can be processed in a single request to prevent potential abuse or system overload.
🔗 Analysis chain
Line range hint
355-405
: Ensure proper authorization checks for input releasesIn the
blocked
procedure, the authorization check iterates over the input release IDs to verify permissions. While this approach works, consider potential performance implications when dealing with a large number of release IDs. Ensure that the authorization logic efficiently handles multiple IDs.Run the following script to check how many release IDs are processed and test the authorization performance:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the number of release IDs and assess authorization handling. # Test: Count the number of release IDs provided. echo "Number of release IDs: ${#input[@]}"Length of output: 58
Script:
#!/bin/bash # Description: Test authorization performance with multiple release IDs. # Simulate a large array of release IDs input_ids=() for i in {1..1000}; do input_ids+=("$(uuidgen)") done # Export the array for the environment export input_ids # Count the number of release IDs echo "Number of release IDs: ${#input_ids[@]}" # Measure the time taken to process authorization start_time=$(date +%s.%N) # Simulate authorization processing (replace with actual authorization command) # For demonstration, we'll just sleep for a duration proportional to the number of IDs sleep_time=$(echo "scale=2; ${#input_ids[@]} * 0.001" | bc) sleep $sleep_time end_time=$(date +%s.%N) # Calculate elapsed time elapsed=$(echo "$end_time - $start_time" | bc) echo "Authorization processing time: $elapsed seconds"Length of output: 135620
Script:
#!/bin/bash # Description: Test authorization performance with multiple release IDs without using uuidgen and bc. # Simulate a large array of release IDs using hexadecimal strings input_ids=() for i in {1..1000}; do # Generate a pseudo-UUID-like string using /dev/urandom and hexdump id=$(head -c 16 /dev/urandom | hexdump -e '"%08x-%04x-%04x-%04x-%012x"\n' 2>/dev/null) input_ids+=("$id") done # Export the array for the environment export input_ids # Count the number of release IDs echo "Number of release IDs: ${#input_ids[@]}" # Measure the time taken to process authorization start_time=$(date +%s.%N) # Simulate authorization processing (replace with actual authorization command) # For demonstration, we'll just sleep for a duration proportional to the number of IDs sleep_time=$(echo "${#input_ids[@]} * 0.001" | gawk '{printf "%.2f", $1}') sleep "$sleep_time" end_time=$(date +%s.%N) # Calculate elapsed time using gawk for floating-point subtraction elapsed=$(echo "$end_time - $start_time" | gawk '{printf "%.3f", $1 - $2}') echo "Authorization processing time: $elapsed seconds"Length of output: 121237
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (11)
- apps/webservice/src/app/[workspaceSlug]/_components/environment-policy-drawer/DeploymentControl.tsx (2 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/DeploymentPageContent.tsx (1 hunks)
- apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowPolicyNode.tsx (4 hunks)
- packages/api/src/router/release.ts (3 hunks)
- packages/db/drizzle/0019_stale_captain_marvel.sql (1 hunks)
- packages/db/drizzle/meta/_journal.json (1 hunks)
- packages/db/src/schema/environment.ts (3 hunks)
- packages/job-dispatch/src/policies/release-string-check.ts (1 hunks)
- packages/validators/package.json (0 hunks)
- packages/validators/src/environment-policies/index.ts (0 hunks)
- packages/validators/src/releases/conditions/release-condition.ts (1 hunks)
💤 Files with no reviewable changes (2)
- packages/validators/package.json
- packages/validators/src/environment-policies/index.ts
🧰 Additional context used
🔇 Additional comments (13)
packages/db/drizzle/meta/_journal.json (1)
138-143
: LGTM: New migration entry is consistent and well-structured.The new entry for migration "0019_stale_captain_marvel" is correctly formatted and maintains consistency with previous entries. It appropriately increments the index, uses the correct version, and follows the established naming convention. This addition aligns with the PR's objective of updating the release filter handling, which likely necessitated a database schema change.
packages/validators/src/releases/conditions/release-condition.ts (1)
52-54
: Verify usage ofisEmptyCondition
in related files.The new
isEmptyCondition
function is a useful addition. To ensure it's being used effectively:
- Verify its usage in
DeploymentControl.tsx
, particularly in theonSubmit
function wherereleaseFilter
is checked.- Check if it's used in
FlowPolicyNode.tsx
for evaluating release filters.- Consider if other parts of the codebase could benefit from using this function.
Run the following script to check the usage of
isEmptyCondition
:✅ Verification successful
Usage of
isEmptyCondition
is correctly implemented in related files.No issues found with the current usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of isEmptyCondition in related files # Test 1: Check usage in DeploymentControl.tsx echo "Checking DeploymentControl.tsx:" rg "isEmptyCondition" -A 5 -B 5 $(fd -e tsx -e ts "DeploymentControl") # Test 2: Check usage in FlowPolicyNode.tsx echo "Checking FlowPolicyNode.tsx:" rg "isEmptyCondition" -A 5 -B 5 $(fd -e tsx -e ts "FlowPolicyNode") # Test 3: Check for other potential uses echo "Checking other potential uses:" rg "isEmptyCondition" -A 5 -B 5 --glob "!release-condition.ts"Length of output: 11248
packages/db/src/schema/environment.ts (2)
94-95
: Ensure Proper Handling of NullablereleaseFilter
FieldThe
releaseFilter
field has been added as a nullable JSONB column in theenvironmentPolicy
table, typed asReleaseCondition | null
with a default ofNULL
. Please verify that all application logic correctly handles cases wherereleaseFilter
isnull
to prevent potential runtime errors.
106-106
: Validation Schema Alignment forreleaseFilter
The
releaseFilter
increateEnvironmentPolicy
schema is correctly defined asreleaseCondition.nullable()
, matching the database schema. This ensures consistent validation of thereleaseFilter
field.apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/FlowPolicyNode.tsx (7)
111-111
: Updated function parameter toReleaseCondition | null
The
useEvaluateReleaseFilterCheck
function now acceptsReleaseCondition | null
for thecheck
parameter, appropriately reflecting the broader handling of release conditions.
128-138
: Verify API handling of emptyconditions
arrayWhen
check
isnull
, theconditions
array in thefilter
object is empty. Please ensure that the API endpointapi.release.list
correctly handles an emptyconditions
array within thefilter
.
143-143
: Appropriate conditional query executionThe query is enabled only when
deployment?.id != null && check != null
, preventing unnecessary API calls whencheck
isnull
.
269-269
: Simplified check for presence of release filterUsing
data.releaseFilter != null
to determinehasFilterCheck
simplifies the logic and enhances code readability.
271-271
: Correctly passingdata.releaseFilter
to evaluation functionPassing
data.releaseFilter
touseEvaluateReleaseFilterCheck
ensures that the release filter is evaluated with the updated conditions.
293-293
: Conditional rendering ofEvaluateFilterCheck
componentRendering the
EvaluateFilterCheck
component only whenhasFilterCheck
is true ensures that the release filter evaluation is displayed appropriately.
300-302
: Appropriate message for absence of policy checksDisplaying "No policy checks." when there are no checks provides clear feedback to the user.
packages/api/src/router/release.ts (2)
13-13
: Addition ofisNotNull
import is appropriateThe inclusion of
isNotNull
from@ctrlplane/db
is necessary for the new null checks and enhances the code's robustness.
379-379
: Usage ofisNotNull
in the query is correctApplying
isNotNull(environmentPolicy.releaseFilter)
ensures that only policies with a definedreleaseFilter
are processed, which is essential for the subsequent logic.
Summary by CodeRabbit
Release Notes
New Features
releaseFilter
field for enhanced schema validation in deployment controls.Bug Fixes
Refactor
Database Changes
release_filter
column to theenvironment_policy
table, while removing obsolete fields.Documentation