fix: Rename resource tables #215
Conversation
WalkthroughThis pull request implements a comprehensive refactor of the codebase, transitioning from a target-centric model to a resource-centric model. Key changes include renaming parameters and variables from Changes
Possibly related PRs
Suggested reviewers
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 60
🧹 Outside diff range and nitpick comments (62)
packages/db/src/schema/target-session.ts (1)
Update Required: Incomplete Refactoring of
targetIdReferencesSeveral remaining references to
targetIdandtarget_idwere found across multiple modules, indicating incomplete refactoring. Please ensure all instances are updated to maintain consistency with the resource-centric model.🔗 Analysis chain
Line range hint
6-13: LGTM: Schema changes align with the resource-centric modelThe table and column renames from
target_sessiontoresource_sessionandtargetIdtoresourceIdare consistent with the PR's objective of transitioning to a resource-centric model. The references and constraints are properly maintained.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify no remaining references to the old table name rg -l "target_session" # Check for any remaining references to targetId in SQL migrations or schema files fd -e sql -e ts | xargs rg "targetId|target_id"Length of output: 16233
packages/db/src/schema/target-group.ts (1)
Old type and helper function references still exist after renaming schema.
- The following files still reference
TargetMetadataGroup:
packages/auth/src/utils/rbac.tspackages/validators/src/auth/index.tspackages/db/src/schema/target-group.tspackages/api/src/router/target-metadata-group.tsapps/webservice/src/app/[workspaceSlug]/(targets)/target-metadata-groups/TargetGroupTable.tsxapps/webservice/src/app/[workspaceSlug]/(targets)/target-metadata-groups/page.tsxapps/webservice/src/app/[workspaceSlug]/(targets)/target-metadata-groups/TargetMetadataGroupsGettingStarted.tsxapps/webservice/src/app/[workspaceSlug]/(targets)/target-metadata-groups/[groupId]/page.tsxapps/webservice/src/app/[workspaceSlug]/(targets)/target-metadata-groups/EditMetadataGroupDialog.tsx- The helper functions
createTargetMetadataGroupandupdateTargetMetadataGroupare still referenced in:
packages/api/src/router/target-metadata-group.tspackages/db/src/schema/target-group.ts🔗 Analysis chain
Line range hint
8-33: Verify the impact of schema renaming across the codebase.The table renaming from
target_metadata_grouptoresource_metadata_grouprequires verification of database migrations and usage across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to the old table name and type names # Check for old table name references echo "Checking for old table name references..." rg -l "target_metadata_group" # Check for old type name references echo "Checking for old type name references..." rg -l "TargetMetadataGroup" # Check for helper function references echo "Checking for helper function references..." rg -l "createTargetMetadataGroup|updateTargetMetadataGroup" # Look for potential migration files echo "Checking for migration files..." fd -g "*migration*.ts" -g "*migration*.sql"Length of output: 3115
Script:
#!/bin/bash # Description: Re-attempt to find migration files with corrected search patterns and verify their contents # Check for migration files with corrected patterns echo "Re-checking for migration files with corrected patterns..." fd -e ts -e sql 'migration' . || echo "No migration files found." # If migration files are found, display their contents related to the table renaming echo "Displaying contents of migration files related to 'resource_metadata_group'..." rg "resource_metadata_group" $(fd 'migration' -e ts -e sql)Length of output: 1990
packages/events/src/triggers/deployment-deleted.ts (3)
Line range hint
2-3: Update type imports to match the resource-centric modelThe imports still use target-centric naming which is inconsistent with the resource-centric model transition:
TargetConditionfrom "@ctrlplane/validators/targets"TargetFilterTypefrom "@ctrlplane/validators/targets"These types should be renamed to maintain consistency with the resource-centric model. Consider updating the imports to use resource-centric naming:
-import type { TargetCondition } from "@ctrlplane/validators/targets"; +import type { ResourceCondition } from "@ctrlplane/validators/resources"; -import { TargetFilterType } from "@ctrlplane/validators/targets"; +import { ResourceFilterType } from "@ctrlplane/validators/resources";Also applies to: 8-9
Line range hint
27-31: Update type annotation to match resource-centric modelThe
systemFilteris still typed asTargetConditionand usesTargetFilterType.Apply this change to maintain consistency:
-const systemFilter: TargetCondition = { - type: TargetFilterType.Comparison, +const systemFilter: ResourceCondition = { + type: ResourceFilterType.Comparison, operator: ComparisonOperator.Or, conditions: envFilters, };
Line range hint
37-40: Update event action to reflect resource-centric modelThe event action still uses "deployment.target.removed".
Consider updating the event action name:
return targets.map((target) => ({ - action: "deployment.target.removed", + action: "deployment.resource.removed", payload: { deployment, target }, }));apps/webservice/src/app/api/v1/job-agents/[agentId]/jobs/running/route.ts (1)
The review comment is correct and should be implemented
The suggested change aligns with the codebase's migration from a target-centric to resource-centric model, as evidenced by schema migrations and similar patterns in other files. The response mapping should use consistent terminology.
🔗 Analysis chain
Line range hint
47-47: Inconsistent naming in response mappingThe response still uses
targetas the field name while mapping data from theresourcetable. This is inconsistent with the PR's objective of transitioning to a resource-centric model.Apply this fix to maintain consistency:
...row.job, config: row.release_job_trigger, environment: row.environment, - target: row.resource, + resource: row.resource, deployment: row.deployment, release: row.release,Let's verify if this field is used by API consumers:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for API response usage where 'target' field might be accessed rg -g '!*.{log,lock}' -A 3 'jobs/running.*\.then'Length of output: 51
Script:
#!/bin/bash # Search for any occurrences of 'target' in the codebase excluding specific file types rg -g '!*.{log,lock}' 'target' # Additionally, search for 'target' in documentation and test files rg -g '*.md' 'target' rg -g '*.test.ts' 'target'Length of output: 542790
Script:
#!/bin/bash # Search for the specific route file and related files rg -g '!*.{log,lock}' -A 5 'apps/webservice/src/app/api/v1/job-agents/\[agentId\]/jobs/running/route'Length of output: 103
Script:
#!/bin/bash # Search for the route file and any references to 'target' field in response mapping rg -g '!*.{log,lock}' -A 3 'target: row.resource' # Also search for the specific file fd "route.ts" "apps/webservice/src/app/api/v1/job-agents"Length of output: 1384
packages/job-dispatch/src/pending-job-checker.ts (1)
Line range hint
7-8: Consider using a more descriptive parameter nameThe parameter name
wfcould be more descriptive. Consider renaming it toworkflowsorworkflowItemsfor better readability.export const isPassingNoPendingJobsPolicy: ReleasePolicyChecker = async ( db, - wf, + workflows, ) =>packages/events/src/triggers/target-deleted.ts (1)
Inconsistent event action usage found
The event action "deployment.target.removed" is used in multiple files, but "deployment.target.deleted" is also present in
packages/validators/src/events/hooks/index.ts. Please ensure consistent event action naming.🔗 Analysis chain
Line range hint
48-51: Verify if event action name should be updatedThe event action "deployment.target.removed" still uses the target terminology. If there are no backward compatibility concerns, consider updating it to "deployment.resource.removed" to maintain consistency with the resource-centric model.
Let's check for event action usage across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of the event action rg "deployment\.target\.removed" -A 2Length of output: 1641
Script:
#!/bin/bash # Corrected search for usages of the event action rg 'deployment\.target\.removed' -A 2Length of output: 1547
apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/TargetVariableDropdown.tsx (1)
47-47: Consider updating prop name for consistencyWhile the change from
targetVariable.targetIdtotargetVariable.resourceIdaligns with the resource-centric model, the prop nametargetIdstill uses the old terminology. Consider updating it toresourceIdfor consistency.- targetId={targetVariable.resourceId} + resourceId={targetVariable.resourceId}packages/db/src/schema/target-provider.ts (1)
30-33: Consider renaming the variable for consistencyWhile the relations are correctly updated, consider renaming the variable to match the new naming pattern:
-export const resourceProviderRelations = relations( +export const resourceProviderToResourceRelations = relations( resourceProvider, ({ many }) => ({ resources: many(resource) }), );packages/events/src/triggers/environment-deleted.ts (2)
46-46: Consider updating variable names for complete consistencyWhile the schema references have been properly updated to use
resource, consider updating these variable names and types for complete consistency:
removedFromSystemTargets→removedFromSystemResourcesTargetCondition→ResourceCondition- const removedFromSystemTargets = + const removedFromSystemResources = envFilters.length > 0 ? await db .select() .from(SCHEMA.resource) .where( and( SCHEMA.targetMatchesMetadata(db, removedFromSystemFilter), inArray( SCHEMA.resource.id, targets.map((t) => t.id), ), ), ) : targets; - if (removedFromSystemTargets.length === 0) return []; + if (removedFromSystemResources.length === 0) return [];Also applies to: 51-51
Update remaining target-related terminology
Multiple instances of "target" terminology still exist in the codebase that need to be updated for consistency:
- Function
targetMatchesMetadatashould be renamed toresourceMatchesMetadata.- Types
TargetConditionandTargetFilterTypeshould be updated toResourceConditionandResourceFilterTyperespectively.- Event actions like
"deployment.target.removed"should be changed to"deployment.resource.removed".🔗 Analysis chain
Line range hint
1-65: Consider updating remaining target-related terminologyFor complete consistency with the resource-centric model, consider updating these remaining instances:
- The helper function
targetMatchesMetadatacould be renamed toresourceMatchesMetadata- The import
TargetFilterTypecould be updated toResourceFilterType- The event action
"deployment.target.removed"could be changed to"deployment.resource.removed"🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining target-related terminology in the codebase # Search for target-related function names echo "Checking for target-related function names..." rg "targetMatches|TargetFilter|TargetCondition" --type ts # Search for target-related event actions echo "Checking for target-related event actions..." rg "deployment\.target\." --type tsLength of output: 32974
packages/db/drizzle/0037_giant_stardust.sql (1)
1-5: Consider adding timestamp columns and ON DELETE behaviorThe
resource_sessiontable structure could benefit from:
- Audit columns like
created_atandupdated_atfor tracking session lifecycle- Explicit ON DELETE behavior for
resource_idandcreated_by_idto handle cascading deletesCREATE TABLE IF NOT EXISTS "resource_session" ( "id" uuid PRIMARY KEY NOT NULL, "resource_id" uuid NOT NULL, - "created_by_id" uuid NOT NULL + "created_by_id" uuid NOT NULL, + "created_at" timestamp with time zone DEFAULT CURRENT_TIMESTAMP NOT NULL, + "updated_at" timestamp with time zone DEFAULT CURRENT_TIMESTAMP NOT NULL );apps/webservice/src/app/api/v1/target-providers/[providerId]/set/route.ts (2)
Line range hint
68-72: Enhance error message clarityThe error message "Provider not found" could be more specific about what type of provider wasn't found.
- { error: "Provider not found" }, + { error: "Resource provider not found" },
Inconsistent Migration of API Paths and Permissions
API paths still contain "target-providers" and permissions are using
TargetUpdateinstead of the updated "resource" terminology. Verify if these should be updated as part of this migration or preserved for backward compatibility.🔗 Analysis chain
Line range hint
44-47: Verify migration strategy for API paths and permissionsWhile the database schema has been updated to use "resource" terminology, the API path still contains "target-providers" and the permission is still
TargetUpdate. Consider:
- Whether these should be updated as part of this change
- If there's a planned phase for updating API paths and permissions
- If these need to be preserved for backward compatibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistency in API paths and permissions # Check for other API routes using target/resource terminology echo "Checking API routes..." fd . "apps/webservice/src/app/api" -t f -E "*.ts" -x rg -l "target-providers|resource-providers" # Check permission definitions echo "Checking permissions..." rg "Permission\." --type ts -A 1Length of output: 26006
packages/job-dispatch/src/job-variables-deployment/utils.ts (3)
47-47: Consider renaming function for consistencyWhile the table and column references have been updated to use
resourceterminology, the function namegetTargetVariableValuestill uses the old "target" terminology. Consider renaming it togetResourceVariableValuefor consistency.-export const getTargetVariableValue = (tx: Tx, targetId: string, key: string) => +export const getResourceVariableValue = (tx: Tx, resourceId: string, key: string) =>Also applies to: 50-51
70-70: Ensure consistent parameter namingWhile the table references have been updated, the function still uses target-centric parameter names:
targetIdparameter could be renamed toresourceIdtargetFilterparameter could be renamed toresourceFilterSCHEMA.targetMatchesMetadatahelper might need renamingThis would make the API more consistent with the new resource-centric model.
-export const getMatchedTarget = ( - tx: Tx, - targetId: string, - targetFilter: TargetCondition | null, +export const getMatchedResource = ( + tx: Tx, + resourceId: string, + resourceFilter: TargetCondition | null,Also applies to: 73-73
Line range hint
1-89: Consider a more comprehensive rename refactoringWhile the database schema references have been successfully updated from
targettoresource, there's an opportunity to make the API more consistent by:
- Renaming all functions to use
resourceinstead oftarget- Updating parameter names to use
resourceIdinstead oftargetId- Updating type names (e.g.,
TargetCondition) to reflect the new terminologyThis would make the codebase more maintainable and prevent confusion between old and new terminology.
apps/webservice/src/app/api/v1/workspaces/[workspaceId]/targets/identifier/[identifier]/route.ts (5)
Line range hint
26-27: Update permission type to match schema changesThe permission check still uses
Permission.TargetGetdespite the transition to a resource-based model. Consider updating the permission types to maintain consistency with the schema changes.- .perform(Permission.TargetGet) + .perform(Permission.ResourceGet)
Line range hint
44-47: Update error message to reflect resource terminologyThe error message still references "Target" despite the schema change to resources. Update the message to maintain consistency with the new terminology.
- { error: "Target not found" }, + { error: "Resource not found" },
Line range hint
73-74: Update permission type in DELETE endpointSimilar to the GET endpoint, the DELETE permission check should be updated to use resource-based permissions.
- .perform(Permission.TargetDelete) + .perform(Permission.ResourceDelete)
Line range hint
88-91: Update error message in DELETE endpointThe error message in the DELETE endpoint should also be updated to use resource terminology.
- { error: `Target not found for identifier: ${params.identifier}` }, + { error: `Resource not found for identifier: ${params.identifier}` },
Update Required in Documentation and Tests
Changing the route from
/targetsto/resourceswill require corresponding updates in the documentation and test files to ensure consistency and prevent broken links or tests.🔗 Analysis chain
Line range hint
1-99: Consider updating the route pathThe route is still under the
/targetspath despite the transition to resources. This might need to be addressed in a separate PR to maintain backward compatibility and handle the migration properly.Let's check for any API documentation or client usage that might be affected:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for API route usage in documentation and test files rg -g '*.{md,mdx}' -i 'targets/identifier' rg -g '*test*' -i 'targets/identifier'Length of output: 81
Script:
#!/bin/bash # Broaden the search for any usage of 'targets' in documentation and test files rg -g '*.{md,mdx}' -i 'targets' rg -g '*test*' -i 'targets'Length of output: 14278
apps/event-worker/src/target-scan/index.ts (1)
Line range hint
43-83: Document terminology transition and update variable namesThe code mixes "target" and "resource" terminology throughout the implementation:
- Variable
tpsuggests "target provider"- Log messages use both terms
- Function names still use "target"
Consider:
- Updating variable names for clarity
- Documenting the terminology transition
- Creating a follow-up task to update related function names
- const tp = await db + const resourceProviderData = await db - logger.error(`Target provider with ID ${targetProviderId} not found.`); + logger.error(`Resource provider with ID ${resourceProviderId} not found.`);packages/job-dispatch/src/environment-creation.ts (1)
86-86: Consider using a more descriptive parameter nameThe mapping function uses 't' as the parameter name. Consider using a more descriptive name like 'resource' for better readability.
- .targets(resources.map((t) => t.id)) + .targets(resources.map((resource) => resource.id))apps/webservice/src/app/api/v1/targets/[targetId]/route.ts (1)
Incomplete target-to-resource transition
The transition from "target" to "resource" is not fully completed. Numerous remaining references to "target" were found across the codebase, which may lead to confusion and inconsistency:
- packages/job-dispatch/src/target.ts
- packages/db/src/schema/target.ts
- packages/job-dispatch/src/release-job-trigger.ts
- packages/validators/src/targets/conditions/index.ts
- apps/webservice/src/app/api/v1/target-providers/[providerId]/route.ts
Please ensure all instances are updated to maintain architectural consistency.
🔗 Analysis chain
Line range hint
1-107: Consider completing the target-to-resource transitionThe codebase appears to be in a transitional state. Several architectural elements still reference the old "target" terminology:
- API route path (
/targets/[targetId])- Permission enums (
TargetGet,TargetUpdate,TargetDelete)- Helper functions (
upsertTargets)This mixed terminology could lead to confusion in the codebase and API documentation.
Let's check for other occurrences of target terminology that might need updating:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for remaining target references in route definitions and permission enums rg -l 'Permission\.Target|\/targets\/|\btarget\b' --type tsLength of output: 188
Script:
#!/bin/bash # Search for remaining target references in route definitions and permission enums rg -l 'Permission\.Target|/targets/|\btarget\b' --type=tsLength of output: 14714
packages/db/src/schema/deployment-variables.ts (1)
63-65: LGTM, but consider full terminology alignmentThe rename from
targetFiltertoresourceFilteraligns with the PR objectives. However, note that the underlying typeTargetConditionand validatortargetConditionstill use the old terminology. Consider creating a follow-up PR to rename these as well for complete consistency.Also applies to: 82-82
packages/api/src/router/workspace-integrations.ts (2)
Line range hint
113-113: Update error message to use resource-centric terminologyThe error message still refers to "target providers" which is inconsistent with the resource-centric model being implemented.
- "Cannot delete Google service account while active target providers exist.", + "Cannot delete Google service account while active resource providers exist.",
Inconsistent terminology: "target provider" found in multiple files.
integrations/google-compute-scanner/src/index.ts: Lines 1-2integrations/terraform-cloud-scanner/src/scanner.ts: Lines 1-2integrations/kubernetes-job-agent/src/utils.ts: Line 1packages/validators/src/events/index.ts: Line 1packages/validators/src/auth/index.ts: Lines 1-3packages/node-sdk/src/schema.ts: Lines 1-8packages/api/src/router/workspace-integrations.ts: Line 113packages/api/src/router/target-provider.ts: Lines 1-20packages/api/src/router/target.ts: Lines 1-5apps/event-worker/src/target-scan/gke.ts: Lines 1-6apps/event-worker/src/target-scan/index.ts: Lines 1-7apps/webservice/src/app/api/v1/target-providers/.../openapi.ts: Lines 1-7apps/webservice/src/app/[workspaceSlug]/.../page.tsx: Multiple linesapps/webservice/src/app/[workspaceSlug]/(job)/job-agents/integrations/page.tsx: Lines 1-4apps/webservice/src/app/[workspaceSlug]/(targets)/.../index.tsx: Multiple linesapps/docs/pages/core-concepts/targets/_meta.ts: Line 1🔗 Analysis chain
Line range hint
113-113: Verify consistent terminology across codebaseLet's verify that all occurrences of "target provider" terminology have been updated to "resource provider" across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of "target provider" terminology rg -i "target\s*provider" --type ts --type tsxLength of output: 76
Script:
#!/bin/bash # Search for any remaining instances of "target provider" in .ts and .tsx files rg -i "target\s*provider" -g "*.ts" -g "*.tsx"Length of output: 23671
apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts (1)
119-120: Update variable names to use consistent resource terminology.While the database query correctly uses the new
resourceMetadatatable, several variable names still use the old "target" terminology.Consider updating the variable names for consistency:
const jobTargetMetadataRows = await db .select() .from(resourceMetadata) - .where(eq(resourceMetadata.resourceId, je.target?.id ?? "")); + .where(eq(resourceMetadata.resourceId, je.resource?.id ?? "")); const metadata = Object.fromEntries( - jobTargetMetadataRows.map((m) => [m.key, m.value]), + resourceMetadataRows.map((m) => [m.key, m.value]), ); - const targetWithMetadata = { ...je.target, metadata }; + const resourceWithMetadata = { ...je.resource, metadata }; return NextResponse.json({ ...je.job, ...je, variables, - target: targetWithMetadata, + resource: resourceWithMetadata, approval, });Also applies to: 122-127, 131-136
apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobTargetConditionRender.tsx (1)
Line range hint
1-150: Consider completing the target-to-resource terminology migration.While the environment filter has been updated to use
resourceFilter, the component still heavily uses target-centric terminology. Consider completing the migration by:
- Renaming the component from
JobTargetConditionRendertoJobResourceConditionRender- Updating imports from
validators/targetsto use resource validators- Updating type names (e.g.,
TargetCondition→ResourceCondition)- Migrating API calls from
api.target.*toapi.resource.*- Updating UI text from "target" to "resource"
This would maintain consistency with the resource-centric model being implemented.
Would you like me to help create a follow-up PR to complete this migration?
packages/job-dispatch/src/policies/release-dependency.ts (1)
92-92: Consider updating the column alias for consistency.While the logic is correct, the column alias
targetIdis still using target-centric terminology despite referencingresourceId. Consider updating it to maintain consistency with the resource-centric model.- targetId: schema.releaseJobTrigger.resourceId, + resourceId: schema.releaseJobTrigger.resourceId,Note: If you make this change, ensure to update all references to
latestJobSubquery.targetIdin the code below.Also applies to: 97-97
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/page.tsx (1)
Line range hint
127-130: Update component description to reflect resource-centric model.The description still refers to "target-specific settings" which doesn't align with the transition to a resource-centric model.
Consider updating the text to:
- Deployment variables allow you to configure target-specific settings + Deployment variables allow you to configure resource-specific settings for your application. Learn more about variable precedence here.packages/api/src/router/workspace.ts (1)
226-229: Consider renaming procedure for consistency with resource-centric modelThe query changes correctly implement the transition to the resource-centric model. However, for better consistency:
- Consider renaming the procedure from
targetKindstoresourceKinds- Update the permission check from
Permission.TargetListto a resource-based permissionHere's a suggested refactor:
- targetKinds: protectedProcedure + resourceKinds: protectedProcedure .meta({ authorizationCheck: ({ canUser, input }) => canUser - .perform(Permission.TargetList) + .perform(Permission.ResourceList) .on({ type: "workspace", id: input }), })apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/_components/variables/VariableInputs.tsx (1)
Line range hint
1-215: Consider a more comprehensive rename strategy.The component and its dependencies still heavily use target-centric terminology (e.g.,
VariableTargetInput,TargetCondition,api.target) while the data model is moving towards a resource-centric approach. This mixed terminology could lead to confusion and maintenance challenges.Consider:
- Renaming this component to
VariableResourceInput- Creating new resource-centric types and gradually deprecating target types
- Adding API endpoints with resource terminology and deprecating target endpoints
- Documenting the migration strategy for other developers
Would you like me to help create a migration plan or generate the renamed component implementation?
apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/CreateTargetVariableDialog.tsx (1)
Refactoring Incomplete: 'target' terminology still present in multiple files.
apps/webservice/src/app/[workspaceSlug]/_components/variable-set-drawer/OverviewContent.tsxapps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobConditionBadge.tsxapps/webservice/src/app/[workspaceSlug]/_components/job-drawer/RelationshipsDiagramDependencies.tsxapps/webservice/src/app/[workspaceSlug]/_components/terminal/TerminalSessionsDrawer.tsxapps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobTargetConditionRender.tsxapps/webservice/src/app/[workspaceSlug]/_components/terminal/TerminalSessionsProvider.tsxapps/webservice/src/app/[workspaceSlug]/_components/job-drawer/JobDrawer.tsxapps/webservice/src/app/[workspaceSlug]/_components/terminal/CreateDialogSession.tsxapps/webservice/src/app/[workspaceSlug]/_components/release-drawer/ReleaseDrawer.tsxapps/webservice/src/app/[workspaceSlug]/_components/target-drawer/CreateTargetVariableDialog.tsx- ... (additional files listed in the shell script output)
🔗 Analysis chain
Line range hint
1-200: Complete the target → resource renaming refactoring.For consistency with the resource-centric model transition, consider updating:
- Component name from
CreateTargetVariableDialogtoCreateResourceVariableDialog- Props interface from
CreateTargetVariableDialogPropsandtargetIdtoCreateResourceVariableDialogPropsandresourceId- UI text in DialogHeader from "Create Target Variable" to "Create Resource Variable"
- File name and location to reflect the new resource-centric model
This will maintain consistency across the codebase and prevent confusion.
Let's verify the broader refactoring pattern:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if other files follow the same renaming pattern echo "Files still using 'target' terminology:" rg -l 'target' --type tsx echo "\nFiles already using 'resource' terminology:" rg -l 'resource' --type tsx echo "\nVerifying mutation usage patterns:" rg -A 2 'mutateAsync\({.*Id:' --type tsxLength of output: 553
Script:
#!/bin/bash # Check if other files follow the same renaming pattern using correct glob pattern echo "Files still using 'target' terminology:" rg 'target' --glob '*.tsx' -l echo "\nFiles already using 'resource' terminology:" rg 'resource' --glob '*.tsx' -l echo "\nVerifying mutation usage patterns with corrected regex:" rg -A 2 'mutateAsync\(\{.*Id:' --glob '*.tsx'Length of output: 16470
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/TableDeployments.tsx (1)
Incomplete resource renaming implementation
Found remaining
targetreferences that need to be updated toresource. Examples include:
apps/event-worker/src/index.tsapps/webservice/src/app/[workspaceSlug]/SidebarWorkspace.tsxapps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/TableDeployments.tsx🔗 Analysis chain
Line range hint
1-4: Incomplete resource renaming implementationThe PR objective is to rename resource tables, but this file still uses target-centric terminology in imports and type definitions. Consider updating these references to align with the resource-centric model:
import type { Deployment, Environment, - Target, + Resource, Workspace, } from "@ctrlplane/db/schema";Let's verify the extent of the renaming needed:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for remaining target references that need to be updated to resource # Search for Target type references ast-grep --pattern 'type { $$$ Target $$$ }' # Search for target-related variable names and properties rg -i '\btarget[s]?\b' --type typescriptLength of output: 124
Script:
#!/bin/bash # Description: Check for remaining target references that need to be updated to resource # Search for Target type references ast-grep --pattern 'type { $$$ Target $$$ }' # Search for target-related variable names and properties rg -i '\btarget[s]?\b' --type ts --type tsxLength of output: 120
Script:
#!/bin/bash # Description: Check for remaining target references that need to be updated to resource # Search for Target type references ast-grep --pattern 'type { $$$ Target $$$ }' # Search for target-related variable names and properties rg -i '\btarget[s]?\b' -g "*.ts" -g "*.tsx"Length of output: 138332
apps/event-worker/src/target-scan/gke.ts (1)
Complete the target → resource renaming transition.
There are numerous remaining "target" related names across various parts of the codebase that need to be updated to ensure consistency and prevent confusion.
- File paths contain "target-scan"
- Multiple type imports, function names, and variables still use "target"
- Log messages and other code references mention "targets"
🔗 Analysis chain
Line range hint
1-300: Consider completing the target → resource renaming transition.While the parameter renaming is correct, there are still many target-related names in the codebase that weren't updated:
- File path contains "target-scan"
- Type imports use "Target" prefix
- Function names and variables use "target"
- Log messages mention "targets"
This partial transition might cause confusion for developers.
Let's verify the scope of remaining target-related names in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for remaining target-related names that might need updating echo "Files with 'target' in path:" fd -t f target echo "\nOccurrences of 'target' in code:" rg -i '\btarget' --type tsLength of output: 247179
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/variables/VariableValueDropdown.tsx (2)
Line range hint
28-31: Consider updating imported validatorsThe imported validators still use target-centric naming which could cause confusion.
Consider updating the imports to use resource-centric naming:
import { defaultCondition, - isValidTargetCondition, - targetCondition, + isValidResourceCondition, + resourceCondition, } from "@ctrlplane/validators/targets"; // Consider renaming this file as well
Line range hint
156-156: Update TargetConditionRender component nameThe
TargetConditionRendercomponent should be renamed to maintain consistency with the resource-centric model.Consider renaming the component and updating its import:
-import { TargetConditionRender } from "~/app/[workspaceSlug]/_components/target-condition/TargetConditionRender"; +import { ResourceConditionRender } from "~/app/[workspaceSlug]/_components/resource-condition/ResourceConditionRender";And its usage:
-<TargetConditionRender +<ResourceConditionRender condition={value ?? defaultCondition} onChange={onChange} />packages/db/src/schema/target.ts (3)
72-82: Consider updating type names for consistencyWhile the table has been renamed to
resource, the type names still use "Target" (e.g.,Target,InsertTarget). Consider renaming these to maintain consistency with the new resource-centric model.-export type Target = InferSelectModel<typeof resource>; -export type InsertTarget = InferInsertModel<typeof resource>; +export type Resource = InferSelectModel<typeof resource>; +export type InsertResource = InferInsertModel<typeof resource>;
Line range hint
200-234: Consider renaming condition-related types and functionsFor consistency with the resource-centric model, consider renaming:
TargetFilterTypetoResourceFilterTypetargetMatchesMetadatatoresourceMatchesMetadata
Line range hint
1-293: Consider a comprehensive type renaming strategyWhile the database schema changes from target to resource are thorough and correct, there's an opportunity to improve the codebase's consistency by updating all related type names. This would involve:
- Creating a migration plan for dependent code
- Updating all type names from
Target*toResource*- Updating related validator types in
@ctrlplane/validators- Maintaining temporary type aliases if needed for backward compatibility
This would complete the transition to a fully resource-centric model.
packages/db/src/schema/environment.ts (1)
41-43: Address naming inconsistency between resource/targetWhile the field has been renamed to
resourceFilter, it still usesTargetConditiontype. This creates an inconsistency in the naming convention. Consider updating the related types and validators for consistency:
TargetCondition→ResourceConditionisValidTargetCondition→isValidResourceConditionpackages/api/src/router/environment.ts (1)
225-230: Consider renaming targetMatchesMetadata functionWhile the code has been updated to use
resourceFilter, thetargetMatchesMetadatafunction name still uses the old terminology. Consider renaming it toresourceMatchesMetadatafor consistency.apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/TargetReleaseTable.tsx (1)
Line range hint
1-476: File name and component should be renamed to align with resource-centric modelBased on the PR objectives to transition from target-centric to resource-centric model, this file needs comprehensive renaming:
- The file should be renamed from
TargetReleaseTable.tsxtoResourceReleaseTable.tsx- The component types and names should be updated:
TargetReleaseTableProps→ResourceReleaseTablePropsTargetReleaseTable→ResourceReleaseTable- Internal variable names should reflect the new terminology:
triggersByTarget→triggersByResourceexpandedTargets→expandedResourcesswitchTargetExpandedState→switchResourceExpandedStatetargets→resourcesWould you like me to generate a comprehensive diff for these changes?
packages/api/src/router/target-provider.ts (1)
Line range hint
158-169: Inconsistent parameter naming:targetProviderIdshould beresourceProviderIdIn the
updateprocedure, the parametertargetProviderIdis still used. Given the shift to a resource-centric model, consider renamingtargetProviderIdtoresourceProviderIdin the input schema and throughout the function for consistency.packages/job-dispatch/src/target.ts (6)
30-30: Consider renaminggetExistingTargetsForProvidertogetExistingResourcesForProviderSince this function now operates on the
resourceentity, renaming it would improve code clarity and consistency with the new model.Apply this diff to rename the function:
-const getExistingTargetsForProvider = (db: Tx, providerId: string) => +const getExistingResourcesForProvider = (db: Tx, providerId: string) =>
Line range hint
33-58: RenamedispatchNewTargetstodispatchNewResourcesThis function now deals with
resourceentities. Renaming it and related variables enhances clarity and consistency.Apply this diff:
-const dispatchNewTargets = async (db: Tx, newTargets: Target[]) => { +const dispatchNewResources = async (db: Tx, newResources: Resource[]) => { const [firstTarget] = newTargets; - if (firstTarget == null) return; + const [firstResource] = newResources; + if (firstResource == null) return; - const workspaceId = firstTarget.workspaceId; + const workspaceId = firstResource.workspaceId;Update all occurrences of
newTargetstonewResourceswithin the function and adjust any function calls accordingly.
Line range hint
61-108: RenameupsertTargetVariablestoupsertResourceVariablesTo reflect the shift from
targettoresource, renaming this function and its internal variables improves readability and maintainability.Apply this diff:
-const upsertTargetVariables = async ( +const upsertResourceVariables = async ( tx: Tx, targets: Array< Target & { variables?: Array<{ key: string; value: any; sensitive: boolean }>; } >, ) => { - const existingTargetVariables = await tx + const existingResourceVariables = await tx .select() .from(resourceVariable) .where( inArray( resourceVariable.resourceId, - targets.map((t) => t.id), + resources.map((r) => r.id), ), ) .catch((err) => { - log.error("Error fetching existing target metadata", { error: err }); + log.error("Error fetching existing resource variables", { error: err }); throw err; });Ensure to update all references to
targetstoresourceswithin the function.
Line range hint
110-149: RenameupsertTargetMetadatatoupsertResourceMetadataUpdating the function name and related variables ensures consistency with the new
resourcemodel.Apply this diff:
-const upsertTargetMetadata = async ( +const upsertResourceMetadata = async ( tx: Tx, targets: Array<Target & { metadata?: Record<string, string> }>, ) => { - const existingTargetMetadata = await tx + const existingResourceMetadata = await tx .select() .from(resourceMetadata) .where( inArray( resourceMetadata.resourceId, - targets.map((t) => t.id), + resources.map((r) => r.id), ), ) .catch((err) => { - log.error("Error fetching existing target metadata", { error: err }); + log.error("Error fetching existing resource metadata", { error: err }); throw err; });Update all references to
targetstoresourceswithin the function to maintain consistency.
Line range hint
201-327: RenameupsertTargetstoupsertResourcesand update related variablesConsidering the shift from
targettoresource, renaming this function and internal variables enhances code clarity and aligns with the data model.Apply this diff:
-export const upsertTargets = async ( +export const upsertResources = async ( tx: Tx, - targetsToInsert: Array< + resourcesToInsert: Array< InsertTarget & { metadata?: Record<string, string>; variables?: Array<{ key: string; value: any; sensitive: boolean }>; } >, ) => { try { log.info("Upserting targets", { - targetsToInsertCount: targetsToInsert.length, + resourcesToInsertCount: resourcesToInsert.length, }); - const targetsBeforeInsertPromises = _.chain(targetsToInsert) + const resourcesBeforeInsertPromises = _.chain(resourcesToInsert) .groupBy((t) => t.providerId) .filter((t) => t[0]?.providerId != null) .map(async (targets) => { const providerId = targets[0]?.providerId;Continue updating all variables from
targets...toresources...throughout the function, includingtargetsBeforeInsert,newTargets,targetsToDelete, and so on.
Line range hint
87-87: Update log messages to reflectresourceinstead oftargetThroughout the code, error log messages still reference
targetinstead ofresource. For consistency and clarity, please update log messages to reflect the new terminology.Apply this diff:
-log.error("Error fetching existing target metadata", { error: err }); +log.error("Error fetching existing resource metadata", { error: err }); -log.error("Error inserting target variables", { error: err }); +log.error("Error inserting resource variables", { error: err }); -log.error("Error deleting target variables", { error: err }); +log.error("Error deleting resource variables", { error: err }); -log.error("Error inserting target metadata", { error: err }); +log.error("Error inserting resource metadata", { error: err }); -log.error("Error deleting target metadata", { error: err }); +log.error("Error deleting resource metadata", { error: err }); -log.error("Error inserting targets", { error: err }); +log.error("Error inserting resources", { error: err }); -log.error("Error upserting targets", { error: err }); +log.error("Error upserting resources", { error: err }); -log.error("Error dispatching new targets", { error: err }); +log.error("Error dispatching new resources", { error: err }); -log.error("Error deleting targets", { error: err }); +log.error("Error deleting resources", { error: err });Ensure all log messages throughout the file use
resourceto maintain consistency.Also applies to: 113-113, 129-129, 147-147, 173-173, 195-195, 257-257, 271-271, 320-320
packages/auth/src/utils/rbac.ts (1)
Line range hint
406-421: Update scope handlers to match renamed functionsAfter renaming the functions, update the
scopeHandlersmapping to use the new function names. For example, change:-resource: getTargetScopes, -resourceView: getTargetViewScopes, -resourceProvider: getTargetProviderScopes, +resource: getResourceScopes, +resourceView: getResourceViewScopes, +resourceProvider: getResourceProviderScopes,packages/api/src/router/deployment.ts (4)
27-27: Consider renaming 'targetMatchesMetadata' to 'resourceMatchesMetadata' for consistencyThe function
targetMatchesMetadatais still in use after the transition fromtargettoresource. Renaming it toresourceMatchesMetadatawould enhance consistency across the codebase and improve readability.
197-197: Rename variable 'targetId' to 'resourceId' for clarityThe property
targetIdis assigned the valuereleaseJobTrigger.resourceId. To reflect the updated terminology fromtargettoresource, consider renamingtargetIdtoresourceId.
475-475: Update property name from 'targetId' to 'resourceId'The property
targetIdis set torow.resource.id. Renaming it toresourceIdwill maintain consistency with the rest of the codebase that now uses the termresource.
443-443: Function name 'targetMatchesMetadata' may need updatingThe function
targetMatchesMetadatais used in conjunction withresourceFilter. For consistency, consider renaming the function toresourceMatchesMetadata.packages/api/src/router/target.ts (4)
Line range hint
186-190: Ensureschema.targetMatchesMetadatais compatible withresource.In this section,
schema.targetMatchesMetadatais used withschema.resource. Verify that this function operates correctly with theresourceschema or consider renaming it toschema.resourceMatchesMetadatato reflect the new naming convention and prevent potential mismatches.
277-304: Consider renamingtargetQuerytoresourceQueryfor consistency.To maintain consistency with the resource-centric model, renaming the
targetQueryfunction toresourceQuerywould enhance code readability and prevent confusion.
304-307: RenametargetRoutertoresourceRouterfor clarity.Given the shift from a target-centric model to a resource-centric model, renaming
targetRoutertoresourceRouterwill align the naming conventions throughout the codebase and improve overall maintainability.
Line range hint
322-332: Update variable names to reflect the resource-centric model.Within the
byIdprocedure, consider renaming variables such astto more descriptive names likeresourceto enhance code clarity and align with the new resource-focused terminology.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (56)
apps/event-worker/src/target-scan/gke.ts(3 hunks)apps/event-worker/src/target-scan/index.ts(2 hunks)apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/Filter.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobTargetConditionRender.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/CreateTargetVariableDialog.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/EditTargetVariableDialog.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/TargetVariableDropdown.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/SystemActionsDropdown.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/_components/variables/VariableInputs.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/TableDeployments.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/page.tsx(4 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/TargetReleaseTable.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/variables/VariableValueDropdown.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/variables/page.tsx(4 hunks)apps/webservice/src/app/api/v1/job-agents/[agentId]/jobs/running/route.ts(2 hunks)apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts(3 hunks)apps/webservice/src/app/api/v1/target-providers/[providerId]/set/route.ts(2 hunks)apps/webservice/src/app/api/v1/targets/[targetId]/route.ts(3 hunks)apps/webservice/src/app/api/v1/workspaces/[workspaceId]/target-providers/name/[name]/route.ts(2 hunks)apps/webservice/src/app/api/v1/workspaces/[workspaceId]/targets/identifier/[identifier]/route.ts(5 hunks)packages/api/src/router/deployment-variable.ts(10 hunks)packages/api/src/router/deployment.ts(8 hunks)packages/api/src/router/environment.ts(3 hunks)packages/api/src/router/job.ts(6 hunks)packages/api/src/router/release-deploy.ts(2 hunks)packages/api/src/router/release.ts(2 hunks)packages/api/src/router/target-metadata-group.ts(7 hunks)packages/api/src/router/target-provider.ts(7 hunks)packages/api/src/router/target.ts(19 hunks)packages/api/src/router/workspace-integrations.ts(2 hunks)packages/api/src/router/workspace.ts(2 hunks)packages/auth/src/utils/rbac.ts(6 hunks)packages/db/drizzle/0037_giant_stardust.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/deployment-variables.ts(2 hunks)packages/db/src/schema/environment.ts(2 hunks)packages/db/src/schema/job.ts(2 hunks)packages/db/src/schema/release.ts(2 hunks)packages/db/src/schema/target-agent.ts(1 hunks)packages/db/src/schema/target-group.ts(2 hunks)packages/db/src/schema/target-provider.ts(3 hunks)packages/db/src/schema/target-session.ts(1 hunks)packages/db/src/schema/target.ts(9 hunks)packages/events/src/triggers/deployment-deleted.ts(2 hunks)packages/events/src/triggers/environment-deleted.ts(3 hunks)packages/events/src/triggers/target-deleted.ts(1 hunks)packages/job-dispatch/src/__test__/job-variables-deployment.test.ts(11 hunks)packages/job-dispatch/src/environment-creation.ts(3 hunks)packages/job-dispatch/src/job-variables-deployment/job-variables-deployment.ts(2 hunks)packages/job-dispatch/src/job-variables-deployment/utils.ts(4 hunks)packages/job-dispatch/src/lock-checker.ts(2 hunks)packages/job-dispatch/src/pending-job-checker.ts(3 hunks)packages/job-dispatch/src/policies/gradual-rollout.ts(1 hunks)packages/job-dispatch/src/policies/release-dependency.ts(3 hunks)packages/job-dispatch/src/release-job-trigger.ts(4 hunks)packages/job-dispatch/src/target.ts(13 hunks)
🔇 Additional comments (96)
packages/db/src/schema/target-agent.ts (1)
5-9: LGTM! Table definition changes are consistent
The changes correctly implement the transition from target to resource:
- Table renamed from "target_session" to "resource_session"
- Foreign key column renamed from targetId to resourceId
- Reference updated to point to resource.id
packages/job-dispatch/src/lock-checker.ts (1)
2-2: LGTM: Import statement correctly updated
The import statement has been properly updated to use resource instead of target, maintaining consistency with the new resource-centric model.
packages/db/src/schema/target-group.ts (1)
Line range hint 8-19: LGTM! Table definition correctly renamed to follow resource-centric model.
The table definition has been properly renamed from target_metadata_group to resource_metadata_group while maintaining all the existing schema structure and constraints.
packages/events/src/triggers/deployment-deleted.ts (1)
33-35: LGTM: Database query updated to use resource table
The change from db.query.target to db.query.resource is correct and aligns with the resource-centric model.
Let's verify the schema helper function has been updated:
#!/bin/bash
# Description: Verify that the targetMatchesMetadata helper has been renamed
# Expected: Should find updated references to resourceMatchesMetadata
# Check for the helper function
rg "Matches.*etadata.*=.*" "@ctrlplane/db/schema"apps/webservice/src/app/api/v1/workspaces/[workspaceId]/target-providers/name/[name]/route.ts (3)
38-42: LGTM! Database operations correctly updated
The database operations have been properly updated to use resourceProvider while maintaining the existing functionality.
Line range hint 1-48: File path needs to be updated for consistency
The file is still located under the target-providers directory despite the transition to resource-centric naming. Consider moving this file to resource-providers to maintain consistency.
#!/bin/bash
# Check for other files/references using the old path pattern
rg -l "target-providers/name/\[name\]"
# Check for any remaining "target-provider" references in the codebase
rg -l "target-provider"This change will require:
- Moving the file to the new location
- Updating any references to this API endpoint
- Ensuring proper handling of backward compatibility if this is a public API
Would you like assistance in creating a migration plan for this change?
7-8: Verify permission enum naming consistency
While the schema import has been updated to use resourceProvider, the Permission.TargetGet enum still uses the old "Target" prefix. This should be updated to maintain consistency with the resource-centric naming.
✅ Verification successful
Permission enum naming is consistent
All instances of Permission.Target have been updated to Permission.Resource.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining "Target" prefixes in permission enums
rg -l "enum.*Permission.*Target"
Length of output: 128
Script:
#!/bin/bash
# Check for any remaining "Target" prefixes in permission enums
rg 'enum.*Permission.*Target'
Length of output: 31
apps/webservice/src/app/api/v1/job-agents/[agentId]/jobs/running/route.ts (2)
12-12: LGTM: Schema import updated correctly
The import change from target to resource aligns with the PR objective of transitioning to a resource-centric model.
30-30: LGTM: Database join updated correctly
The left join operation has been properly updated to use the new resource table instead of the previous target table.
packages/job-dispatch/src/pending-job-checker.ts (2)
15-15: LGTM: Consistent schema field renaming
The change from targetId to resourceId in the selectDistinctOn clause is consistent with the resource-centric model transition.
49-49: LGTM: Consistent filter condition update
The filter predicate correctly uses resourceId for comparison, maintaining the logical integrity of the triple-key match.
packages/events/src/triggers/target-deleted.ts (1)
29-31: Update type definitions to match resource-centric model
The filter mapping has been updated, but the types TargetCondition and TargetFilterType (used below) still reference the old target-centric model.
Let's check if these types have been updated in their definition files:
packages/db/src/schema/target-provider.ts (3)
Line range hint 17-29: Verify database migration for table renaming
The table definition looks correct, but ensure there's a corresponding migration script to handle the rename from "target_provider" to "resource_provider".
Line range hint 40-62: Update remaining target references and verify constraints
- The table and foreign key changes look good, but function and type names need updating for consistency:
-export const createTargetProviderGoogle = createInsertSchema(
+export const createResourceProviderGoogle = createInsertSchema(
resourceProviderGoogle,
{ projectIds: z.array(z.string().min(1)).min(1) },
).omit({ id: true });
-export const updateTargetProviderGoogle = createTargetProviderGoogle.partial();
+export const updateResourceProviderGoogle = createResourceProviderGoogle.partial();
-export type TargetProviderGoogle = InferSelectModel<
+export type ResourceProviderGoogle = InferSelectModel<
typeof resourceProviderGoogle
>;- Verify the foreign key constraint changes:
14-14: Verify the import path consistency
The import path ./target.js seems inconsistent with the resource-centric model transition.
packages/events/src/triggers/environment-deleted.ts (2)
22-22: LGTM: Schema reference correctly updated
The schema reference has been properly updated from targetFilter to resourceFilter, maintaining consistency with the renaming pattern.
32-32: LGTM: Environment filter mapping updated correctly
The environment filter mapping has been properly updated to use resourceFilter, maintaining the same functionality while following the new naming convention.
packages/db/drizzle/0037_giant_stardust.sql (4)
7-8: LGTM! Column renames are consistent with the resource-centric model
The column renames from target_id to resource_id are properly ordered and align with the PR objectives.
39-40: LGTM! Indexes are properly defined
The unique composite indexes are well-structured with:
- Appropriate use of IF NOT EXISTS
- Correct column ordering for the use case
15-37:
Review ON DELETE behavior consistency
There's an inconsistency in ON DELETE behavior across foreign key constraints:
resource_session: ON DELETE no actionresource_metadataandresource_variable: ON DELETE cascade
Please verify if this difference is intentional, as it could lead to unexpected behavior when deleting resources.
9-14: Verify data integrity before applying migration
While the constraint and index drops are properly ordered, ensure that existing data maintains referential integrity before applying this migration.
apps/webservice/src/app/api/v1/target-providers/[providerId]/set/route.ts (1)
62-64: LGTM! Database query changes are consistent
The changes to use resourceProvider in the database query and join conditions are correctly implemented.
packages/job-dispatch/src/job-variables-deployment/utils.ts (1)
32-33: Verify schema migration for target to resource table rename
The table reference change from SCHEMA.target to SCHEMA.resource looks correct. However, we should ensure that the corresponding schema migration exists and handles data transfer appropriately.
apps/webservice/src/app/api/v1/workspaces/[workspaceId]/targets/identifier/[identifier]/route.ts (2)
17-20: LGTM: Database queries correctly updated to use resource schema
The database queries have been properly updated to use the new resource schema while maintaining the same query structure and conditions.
Also applies to: 32-35
94-94: LGTM: Resource deletion correctly implemented
The deletion operation has been properly updated to use the resource schema.
apps/event-worker/src/target-scan/index.ts (1)
Line range hint 1-83: Verify event handling compatibility
Since this file handles events and the event channel is still named TargetScan, verify that the event handling system (producers/consumers) is compatible with these changes.
#!/bin/bash
# Check for event definitions and usage
rg -A 5 "Channel\.TargetScan" --type ts
rg -A 5 "TargetScanEvent" --type ts
# Check for any queue consumers/producers
rg -A 5 "targetScanQueue" --type tspackages/job-dispatch/src/environment-creation.ts (4)
17-18: LGTM: Variable renaming is consistent with resource-centric model
The renaming from targetFilter to resourceFilter aligns well with the broader refactoring effort.
49-49: LGTM: Consistent variable renaming
The change from targets to resources maintains the same logic while aligning with the new terminology.
86-86: Verify method chain rename consistency
The method chain still uses .targets() instead of .resources(). This might indicate a missed rename in the builder pattern.
40-46: Verify schema function rename consistency
While the table has been renamed from target to resource, the schema function targetMatchesMetadata still uses the old terminology. This might indicate a missed rename.
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/variables/page.tsx (3)
Line range hint 44-53: LGTM: Consistent property renaming
The changes correctly update the null check and filter hash generation to use resourceFilter instead of targetFilter, maintaining the same logical flow.
59-59: LGTM: Filter condition updated correctly
The filter condition correctly uses the new resourceFilter property while maintaining the existing filter logic.
79-79: LGTM: Rest filters mapping updated appropriately
The mapping of rest filters correctly uses the new resourceFilter property while preserving the existing default value handling logic.
packages/api/src/router/workspace-integrations.ts (2)
8-9: LGTM! Import changes align with resource-centric model
The schema imports have been correctly updated to reflect the transition from target-centric to resource-centric model.
101-106: LGTM! Query changes correctly implement resource-centric model
The database query has been properly updated to use the new resource-centric schema tables and join conditions.
packages/job-dispatch/src/job-variables-deployment/job-variables-deployment.ts (2)
37-37: 🛠️ Refactor suggestion
Maintain consistency in resource-centric naming
While the ID parameter has been renamed to resourceId, the surrounding code still uses target-centric terminology. For complete alignment with the resource-centric model, consider renaming:
getTargetutility function togetResourcejobTargetvariable tojobResource
Line range hint 1-170: Consider comprehensive resource-centric renaming
While the immediate changes align with the resource-centric model, there are several other occurrences of target-centric terminology throughout the file that should be updated for consistency:
-
Type definitions:
schema.Targetshould becomeschema.ResourceTargetVariableshould becomeResourceVariable
-
Function names and parameters:
getTargetVariableValueshould becomegetResourceVariableValuetargetVariableValueshould becomeresourceVariableValue
This would ensure complete alignment with the resource-centric model across the codebase.
apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts (2)
14-15: LGTM: Import changes align with resource table renaming.
The new imports for resource and resourceMetadata correctly reflect the transition from target-centric to resource-centric model.
Line range hint 131-137: Verify API documentation and client compatibility.
The response structure changes from target to resource will be a breaking change for API consumers.
Let's check for API documentation and potential client usage:
packages/api/src/router/release-deploy.ts (2)
12-12: LGTM: Import change aligns with schema refactoring
The addition of the resource import from the schema is consistent with the transition from target to resource-centric model.
Line range hint 91-126: Complete the target-to-resource terminology transition
There are several instances where the old "target" terminology is still being used despite the transition to resources:
- The procedure name is still
toTarget - The input parameter is still
targetId - Permission check uses
TargetUpdatebut referencesresourcetype - Variable name
tcould be more descriptive
Consider applying these changes for consistency:
- toTarget: protectedProcedure
+ toResource: protectedProcedure
.meta({
authorizationCheck: ({ canUser, input }) =>
canUser
- .perform(Permission.ReleaseGet, Permission.TargetUpdate)
+ .perform(Permission.ReleaseGet, Permission.ResourceUpdate)
.on(
{ type: "release", id: input.releaseId },
- { type: "resource", id: input.targetId },
+ { type: "resource", id: input.resourceId },
),
})
.input(
z.object({
- targetId: z.string().uuid(),
+ resourceId: z.string().uuid(),
releaseId: z.string().uuid(),
environmentId: z.string().uuid(),
isForcedRelease: z.boolean().optional(),
}),
)
.mutation(async ({ ctx, input }) => {
- const t = await ctx.db
+ const resource = await ctx.db
.select()
.from(resource)
- .where(eq(resource.id, input.targetId))
+ .where(eq(resource.id, input.resourceId))
.then(takeFirstOrNull);Let's verify if there are any remaining "target" references that need to be updated:
packages/job-dispatch/src/policies/release-dependency.ts (2)
66-66: LGTM! Condition updated correctly.
The relationship exclusion condition has been properly updated to use resourceId.
86-86: LGTM! Array construction updated properly.
The unique ID collection logic has been correctly updated to use resourceId.
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/page.tsx (2)
Line range hint 46-55: LGTM! Consistent resource-centric naming.
The changes to null checking and filter hash generation maintain the same logic while adopting the new resource-centric terminology.
36-36: Consider updating type names for consistency with resource-centric model.
While the change from targetFilter to resourceFilter aligns with the resource-centric transition, the types (TargetFilterType, TargetOperator) still use the old target-centric naming convention.
Let's check if these types are being renamed elsewhere in the codebase:
packages/db/drizzle/meta/_journal.json (2)
263-269: LGTM! Migration entry follows the established pattern.
The new migration entry is properly structured and maintains consistency with existing entries. The sequential indexing and timestamps are correct.
263-269: Verify the migration content for table renaming.
Since this PR involves renaming resource tables, let's verify the actual migration SQL.
✅ Verification successful
Migration Content Verified Successfully.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the migration content for table renaming changes
# Look for the migration file corresponding to this journal entry
fd -t f "0037_giant_stardust" packages/db/drizzle/
# If found, examine its content for table renaming operations
if [ $? -eq 0 ]; then
echo "Migration file content:"
cat $(fd -t f "0037_giant_stardust" packages/db/drizzle/)
fi
# Look for any references to 'target' or 'resource' in SQL files
echo "Searching for target/resource references in migrations:"
rg -t sql "ALTER TABLE.*target|RENAME.*target|ALTER TABLE.*resource|RENAME.*resource" packages/db/drizzle/
Length of output: 15045
packages/api/src/router/workspace.ts (2)
16-16: LGTM: Import changes align with resource-centric model transition
The addition of the resource import is consistent with the codebase's transition from target-centric to resource-centric model.
226-229: Verify Permission enum updates
Let's verify if the Permission enum has been updated to include the new resource-based permissions.
✅ Verification successful
Permission enum updates verified
All Permission.Target* permissions used in the codebase are correctly defined in the Permission enum.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if Permission enum includes resource-based permissions
# Look for Permission enum definition
rg -A 10 "enum Permission" --type ts
# Check for any remaining target-related permissions
rg "Permission\.Target" --type ts
Length of output: 4220
Script:
#!/bin/bash
# Description: Extract the full definition of the Permission enum
rg -A 50 "enum Permission" --type ts packages/validators/src/auth/index.ts
Length of output: 1658
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/_components/variables/VariableInputs.tsx (1)
129-130: LGTM, but terminology is inconsistent.
The change from targetFilter to resourceFilter is correct, but it creates a mix of terminology in the codebase where we're using both "target" and "resource" concepts.
Let's verify the consistency of this change across related files:
packages/db/src/schema/job.ts (1)
221-221: Consider updating JobFilterType enum name for consistency.
While the code correctly uses resource.id, the enum type JobFilterType.JobTarget still references "Target". This seems inconsistent with the resource-centric model transition.
#!/bin/bash
# Description: Find all occurrences of JobFilterType.JobTarget to ensure consistent renaming
# Search for JobFilterType enum definition
ast-grep --pattern 'export const JobFilterType = $$$'
# Search for all uses of JobTarget in enum or type definitions
rg "JobTarget" -A 5 -B 5apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/Filter.tsx (1)
127-127:
Inconsistent terminology between UI and backend
While the backend has transitioned from targetFilter to resourceFilter, the UI components, form fields, and types still use the "target" terminology. This creates a confusing mismatch between the presentation and data layers.
Consider a more comprehensive rename to maintain consistency:
- const filterForm = z.object({
- targetFilter: targetCondition.optional(),
- });
+ const filterForm = z.object({
+ resourceFilter: targetCondition.optional(),
+ });
const form = useForm({
schema: filterForm,
- defaultValues: { targetFilter: getFilter(environment.resourceFilter) },
+ defaultValues: { resourceFilter: getFilter(environment.resourceFilter) },
});
- const { targetFilter } = form.watch();
+ const { resourceFilter } = form.watch();
- const filter = targetFilter ?? undefined;
+ const filter = resourceFilter ?? undefined;
// Update the mutation data
onSubmit.mutateAsync({
id: environment.id,
- data: { ...data, resourceFilter: targetFilter ?? null },
+ data: { ...data, resourceFilter: resourceFilter ?? null },
})Additionally, consider renaming:
- The
TargetViewsComboboxcomponent toResourceViewsCombobox - The
TargetConditionRendercomponent toResourceConditionRender - Update all related prop names and types
Let's verify the extent of the target-to-resource rename across the codebase:
Also applies to: 144-144
✅ Verification successful
Inconsistent terminology between UI and backend needs to be addressed
The review comment correctly identifies that while the backend has transitioned from targetFilter to resourceFilter, the UI form fields still use the old terminology. This should be fixed to maintain consistency with the broader target-to-resource rename throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for remaining "target" references that might need updating
rg -i "target" --type tsx --type ts
# Search for new "resource" terminology to ensure consistency
rg -i "resource" --type tsx --type ts
Length of output: 129
Script:
#!/bin/bash
# Search for remaining "target" references that might need updating
rg -i "target" --glob "*.tsx" --glob "*.ts"
# Search for new "resource" terminology to ensure consistency
rg -i "resource" --glob "*.tsx" --glob "*.ts"
Length of output: 347553
apps/event-worker/src/target-scan/gke.ts (1)
243-243: LGTM! Parameter renaming aligns with resource-centric model.
The changes correctly update the parameter from targetProviderId to resourceProviderId consistently across all function calls.
Also applies to: 275-275, 286-286
packages/db/src/schema/release.ts (1)
188-190: LGTM! Field rename aligns with the resource-centric model.
The renaming of the field from targetId to resourceId and updating its reference to resource.id is consistent with the refactoring goal. The cascade delete constraint is properly maintained.
apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/variables/VariableValueDropdown.tsx (1)
Line range hint 1-1: Verify the impact of renaming across the codebase
Let's ensure all related components and files are updated consistently.
✅ Verification successful
No Impact Found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for remaining target-related terms that need to be updated
# Check for TargetConditionRender component usage
echo "Checking TargetConditionRender component usage:"
rg "TargetConditionRender"
# Check for targetFilter references
echo -e "\nChecking targetFilter references:"
rg "targetFilter"
# Check for target-condition directory
echo -e "\nChecking target-condition directory:"
fd "target-condition"
# Check for validators/targets references
echo -e "\nChecking validators/targets references:"
rg -g "*.{ts,tsx}" "from.*validators/targets"
Length of output: 17533
Script:
#!/bin/bash
# Description: Check if 'schema' is used in files that utilize 'TargetConditionRender' or 'targetFilter'
# Find files that import 'schema'
echo "Searching for 'schema' usage in relevant files:"
fd ".*\.tsx?$" | xargs grep -l "import .*schema"
# Within those files, check for usage of 'TargetConditionRender' or 'targetFilter'
echo -e "\nChecking for 'TargetConditionRender' or 'targetFilter' usage alongside 'schema':"
fd ".*\.tsx?$" | xargs grep -E "schema|TargetConditionRender|targetFilter"
Length of output: 114177
packages/db/src/schema/target.ts (4)
Line range hint 37-61: LGTM: Table definition correctly renamed while maintaining structure
The table definition has been properly renamed from target to resource while maintaining all the necessary columns, constraints, and relationships. The foreign key reference to resourceProvider has been correctly updated.
118-139: LGTM: Metadata table and relations properly renamed
The metadata table and its relations have been correctly renamed while maintaining the existing structure and constraints.
236-293: Update relationship and variable type names
Consider renaming the following types to align with the resource-centric model:
TargetRelationship→ResourceRelationshipTargetVariable→ResourceVariable
Also, consider updating the create/update function names:
createTargetRelationship→createResourceRelationshipcreateTargetVariable→createResourceVariable
#!/bin/bash
# Check for impacts of renaming these types
rg "TargetRelationship|TargetVariable" --type typescript
# Check for any remaining "target" references in schema files
fd "schema" --type f --exec rg "target" --ignore-case {}Line range hint 100-116: Update condition type to match resource naming
The filter field in resourceView still uses the TargetCondition type. Consider updating this type name to maintain consistency with the resource-centric model.
packages/api/src/router/environment.ts (3)
21-21: LGTM: Schema import aligns with resource-centric model transition
The addition of the resource schema import is consistent with the PR's objective of transitioning from target-centric to resource-centric model.
236-244: Verify the workspaceId filter addition
The addition of the workspaceId check in the resource query is a significant change. Please verify that this additional filter:
- Doesn't exclude valid resources that should be included
- Is consistent with how workspace filtering is handled in other similar queries
#!/bin/bash
# Description: Check how workspaceId filtering is implemented across the codebase
# Search for similar resource/target queries to compare workspace filtering
echo "Checking resource/target query patterns:"
ast-grep --pattern 'from(resource)
.where(
$$$
workspaceId
$$$
)'158-164: Verify consistent usage of resource terminology
The query has been updated to use the new resource-centric model, but let's verify that all related references have been updated consistently.
packages/api/src/router/release.ts (3)
29-29: LGTM: Import addition is correct
The addition of the resource import aligns with the schema refactoring from target to resource.
107-107: Verify complete schema migration
The join condition has been correctly updated to use the resource table. Let's verify that all related queries have been updated consistently.
Line range hint 119-119: Breaking Change: API Response Structure
The property name change from target to resource in the response object is a breaking change that will affect API clients. Please ensure that:
- All API clients are updated to handle the new property name
- This change is documented in the API changelog
- The version number is bumped appropriately
packages/job-dispatch/src/__test__/job-variables-deployment.test.ts (5)
47-47: LGTM: Consistent rename from targetId to resourceId
The change aligns with the PR objective of transitioning to a resource-centric model.
81-81: LGTM: Consistent rename of filter properties
The changes to resourceFilter are consistently applied across all variable value definitions.
Also applies to: 87-87, 93-93
240-240: LGTM: Environment resource filter updates
The changes consistently rename the filter property across all environment test fixtures.
Also applies to: 308-308, 375-375, 442-442, 509-509, 604-604, 699-699
571-571: LGTM: Target variable property updates
The resourceId property is consistently updated in both sensitive and non-sensitive target variable test cases.
Also applies to: 666-666
Line range hint 1-761: Verify remaining target references
While the property renames look good, we should verify that no target-related terminology was missed in variable names, comments, or test descriptions.
packages/job-dispatch/src/release-job-trigger.ts (1)
18-18: 🛠️ Refactor suggestion
Rename targetMatchesMetadata to resourceMatchesMetadata for consistency
In line with the refactoring from 'target' to 'resource', the imported function targetMatchesMetadata should be renamed to resourceMatchesMetadata for clarity and consistency.
Apply this diff to update the import:
import {
deployment,
environment,
job,
jobAgent,
release,
releaseJobTrigger,
resource,
system,
- targetMatchesMetadata,
+ resourceMatchesMetadata,
} from "@ctrlplane/db/schema";Likely invalid or redundant comment.
packages/api/src/router/target-provider.ts (9)
30-35: LGTM
The query in the byWorkspaceId procedure correctly updates references to resourceProvider and resourceProviderGoogle, aligning with the new resource-centric model.
41-51: LGTM
The providerCounts query correctly updates to use resource and resourceProvider, ensuring accurate counting of resources per provider.
55-67: LGTM
The providerKinds query is appropriately updated to reference resource and resourceProvider. Grouping by kind and version maintains the integrity of the data retrieval.
94-95: LGTM
The byId procedure correctly fetches the resourceProvider by its ID using the updated entity name.
132-132: LGTM
The insertion into resourceProvider correctly reflects the updated model.
137-138: LGTM
The insertion into resourceProviderGoogle accurately assigns the resourceProviderId and aligns with the new data model.
174-178: Duplicate: Inconsistent parameter naming
As previously mentioned, the use of input.targetProviderId should be updated to input.resourceProviderId to maintain consistency with the new resource-centric terminology.
224-225: LGTM
The deletion of resources associated with the provider correctly uses resource and resource.providerId, aligning with the updated model.
228-229: LGTM
The deletion of the resourceProvider is properly executed using the updated entity names.
packages/job-dispatch/src/target.ts (1)
95-95: Confirm encryption logic for sensitive variables
Ensure that the encryption of sensitive variables using variablesAES256().encrypt(JSON.stringify(value)) complies with security best practices.
Run the following script to verify that all sensitive variables are encrypted consistently:
This script checks for the correct encryption pattern across the codebase.
✅ Verification successful
Encryption logic for sensitive variables is properly implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search the codebase for encryption of sensitive variables.
# Search for encryption usage
rg 'variablesAES256\(\)\.encrypt\(JSON\.stringify\('
Length of output: 149
packages/auth/src/utils/rbac.ts (1)
16-19: Approved: Addition of new resource imports
The import statements for resource, resourceMetadataGroup, resourceProvider, and resourceView are appropriately added and align with the resource-centric refactor.
packages/api/src/router/deployment-variable.ts (8)
24-24: Importing 'resource' aligns with the refactoring
The addition of resource to the imports reflects the shift from a target-centric to a resource-centric model.
121-121: Use of environment.resourceFilter ensures consistency
Updating to environment.resourceFilter maintains consistency with the new resource-based approach.
131-131: Correctly mapping conditions using resourceFilter
The expression dep.map((e) => e.resourceFilter).filter(isPresent) appropriately collects resource filters from environments.
220-220: Confirm targetMatchesMetadata function usage
As previously noted, confirm whether targetMatchesMetadata is appropriate for resources or if a resource-specific function should be used.
330-333: Ensure correct function usage with resources
Confirm that targetMatchesMetadata is suitable when used with resources in this context, or consider using an appropriate alternative.
179-181: 🛠️ Refactor suggestion
Rename functions and variables for clarity and consistency
The function getOldTargetFilter and the use of value.resourceFilter mix 'target' and 'resource' terminology. Consider renaming to getOldResourceFilter and updating related variables to reflect the resource-centric model.
Apply this diff to update the function name:
-const getOldTargetFilter = (): TargetCondition | null => {
+const getOldResourceFilter = (): TargetCondition | null => {And update variable names within the function accordingly.
Likely invalid or redundant comment.
301-302: 🛠️ Refactor suggestion
Rename byTargetId to byResourceId for consistency
The procedure byTargetId operates on resources. Renaming it to byResourceId will enhance clarity and align with the updated terminology.
Apply this diff to rename the procedure:
-export const deploymentVariableRouter = createTRPCRouter({
- value: valueRouter,
-
- byTargetId: protectedProcedure
+export const deploymentVariableRouter = createTRPCRouter({
+ value: valueRouter,
+
+ byResourceId: protectedProcedureLikely invalid or redundant comment.
216-216: Verify the use of targetMatchesMetadata with resources
The function targetMatchesMetadata is used with resources. Ensure this function is intended for resources or replace it with a suitable alternative like resourceMatchesMetadata.
Run the following script to check if a resourceMatchesMetadata function exists:
packages/api/src/router/deployment.ts (1)
429-429: Verify all instances of 'environment.targetFilter' are updated
Ensure that all occurrences of environment.targetFilter have been updated to environment.resourceFilter throughout the codebase to prevent any inconsistencies.
Run the following script to check for any remaining references:
packages/api/src/router/job.ts (5)
47-47: Import Update Aligns with Resource Refactoring
The addition of resource to the imports is appropriate and necessary for the refactoring from target to resource. This ensures that all references to resource are correctly resolved.
69-69: Updated Inner Join to Use 'Resource' Entity
The inner join has been correctly updated to use resource instead of target, aligning with the new data model. This change ensures that the query joins releaseJobTrigger with the resource table based on resourceId.
188-188: Inner Join Updated to Reference 'Resource'
The inner join on resource using releaseJobTrigger.resourceId ensures that the query correctly associates release job triggers with resources. This change is consistent with the shift from target to resource.
437-438: Query Updated to Retrieve Resources
The .from(resource) and .where(inArray(resource.id, allIds)) correctly update the query to select resources based on their IDs. This modification aligns with the refactoring efforts and appears to function as intended.
450-450: Partition Clause Updated with 'Resource' Identifier
The PARTITION BY clause now correctly uses releaseJobTrigger.resourceId in place of targetId, which aligns with the data model changes to use resource. This update ensures correct windowing functions over the new partition key.
| import { pgTable, uuid } from "drizzle-orm/pg-core"; | ||
|
|
||
| import { target } from "./target.js"; | ||
| import { resource } from "./target.js"; |
There was a problem hiding this comment.
💡 Codebase verification
Import path inconsistency detected
- The
resource.jsfile does not exist inpackages/db/src/schema. - Remaining references to
target.jsfound in:packages/db/src/schema/target-session.tspackages/db/src/schema/target-agent.tspackages/db/src/schema/release.tspackages/db/src/schema/target-provider.tspackages/db/src/schema/job.tspackages/db/src/schema/index.ts
Please update the import paths to align with the resource-centric model.
🔗 Analysis chain
Verify the import path naming consistency
The import statement shows resource being imported from target.js, which seems inconsistent with the resource-centric model transition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if target.js has been renamed to resource.js in the codebase
# and if there are any remaining references to the old filename
# Check if resource.js exists
fd -t f "resource.js" "packages/db/src/schema"
# Check for any remaining references to target.js
rg -l "target\.js" "packages/db/src/schema"
Length of output: 306
|
|
||
| import { user } from "./auth.js"; | ||
| import { target } from "./target.js"; | ||
| import { resource } from "./target.js"; |
There was a problem hiding this comment.
💡 Codebase verification
Import Path Inconsistency Detected
The import statement in ./target-session.ts still references ./target.js, indicating incomplete refactoring to the resource-centric model.
🔗 Analysis chain
Verify the import path naming consistency
While we're transitioning to a resource-centric model, we're still importing from "./target.js". This seems inconsistent with the refactoring effort.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the target.js file has been renamed or if there are other files still referencing it
fd target.js
rg -l "from ['\"].*target\.js['\"]"
Length of output: 16233
| .select() | ||
| .from(releaseJobTrigger) | ||
| .innerJoin(target, eq(releaseJobTrigger.targetId, target.id)) | ||
| .innerJoin(resource, eq(releaseJobTrigger.resourceId, resource.id)) |
There was a problem hiding this comment.
💡 Codebase verification
References to 'target' table still found in SQL queries. Please ensure all instances are correctly renamed to 'resource'.
integrations/terraform-cloud-scanner/src/scanner.tspackages/validators/src/session/session-create.tspackages/validators/src/variables/index.tsapps/webservice/src/app/api/github/webhook/workflow/handler.tsapps/webservice/src/app/[workspaceSlug]/_components/reactflow/edges.ts- ...and others.
🔗 Analysis chain
LGTM: Database query properly refactored
The database query has been correctly updated to:
- Join with the
resourcetable instead oftarget - Reference
resource.lockedAtin the null check condition
The changes maintain the same functionality while aligning with the resource-centric model.
Let's verify that all references to the target table have been consistently renamed:
Also applies to: 20-20
[/run_scripts]
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to 'target' table in SQL queries
# Expected: No references to 'target' table in SQL queries after the rename
# Search for any remaining SQL queries using the old 'target' table
rg -g '*.ts' -g '*.js' 'from.*target.*join|join.*target\.'
Length of output: 60
Script:
#!/bin/bash
# Description: Re-verify any remaining references to the 'target' table in SQL queries with an improved search pattern
# Expected: No references to 'target' table in SQL queries after the rename
# Search for any remaining SQL queries using the old 'target' table with a broader regex
rg -i -g '*.ts' -g '*.js' '\btarget\b'
Length of output: 32275
| export type TargetMetadataGroup = InferSelectModel< | ||
| typeof resourceMetadataGroup | ||
| >; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider renaming the type alias for consistency.
The type alias still uses the old 'Target' prefix while inferring from the renamed resourceMetadataGroup.
Apply this diff to maintain consistency:
-export type TargetMetadataGroup = InferSelectModel<
+export type ResourceMetadataGroup = InferSelectModel<
typeof resourceMetadataGroup
>;📝 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.
| export type TargetMetadataGroup = InferSelectModel< | |
| typeof resourceMetadataGroup | |
| >; | |
| export type ResourceMetadataGroup = InferSelectModel< | |
| typeof resourceMetadataGroup | |
| >; |
There was a problem hiding this comment.
I think this is a miss as well?
There was a problem hiding this comment.
@zacharyblasczyk, understood. We'll keep the type alias as TargetMetadataGroup. Thanks for clarifying!
| export const createTargetMetadataGroup = createInsertSchema( | ||
| resourceMetadataGroup, | ||
| ) | ||
| .omit({ | ||
| id: true, | ||
| }) | ||
| .extend({ | ||
| keys: z.array(z.string()), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Maintain naming consistency by renaming helper functions.
While the main table has been renamed to use resource, the helper functions still use Target in their names. This creates inconsistency in the codebase.
Apply this diff to maintain consistency:
-export const createTargetMetadataGroup = createInsertSchema(
+export const createResourceMetadataGroup = createInsertSchema(
resourceMetadataGroup,
)
.omit({
id: true,
})
.extend({
keys: z.array(z.string()),
});
-export const updateTargetMetadataGroup = createTargetMetadataGroup.partial();
+export const updateResourceMetadataGroup = createResourceMetadataGroup.partial();Committable suggestion skipped: line range outside the PR's diff.
| return updatedValue.resourceFilter; | ||
| const conditions = otherValues | ||
| .map((v) => v.targetFilter) | ||
| .map((v) => v.resourceFilter) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update getNewTargetFilter to getNewResourceFilter
For consistency, rename getNewTargetFilter to getNewResourceFilter and adjust variable names to align with the resource-centric changes.
Apply this diff to rename the function:
-const getNewTargetFilter = (): TargetCondition | null => {
+const getNewResourceFilter = (): TargetCondition | null => {And update associated variables within the function.
Committable suggestion skipped: line range outside the PR's diff.
| schema.resource.workspaceId, | ||
| input.workspaceId, | ||
| ); | ||
| const targetConditions = schema.targetMatchesMetadata( |
There was a problem hiding this comment.
Update usage of schema.targetMatchesMetadata in byWorkspaceId procedure.
The schema.targetMatchesMetadata function is again used with schema.resource. Ensure that this function is compatible with the resource schema or update its implementation and name to align with the resource-centric model.
| .select({ | ||
| id: releaseJobTrigger.id, | ||
| targetId: releaseJobTrigger.targetId, | ||
| targetId: releaseJobTrigger.resourceId, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Property Name Inconsistency with 'Resource' Refactoring
The property targetId is assigned the value releaseJobTrigger.resourceId. To maintain consistency and prevent confusion, consider renaming the property to resourceId.
Apply this diff to update the property name:
const latestJobSubquery = ctx.db
.select({
id: releaseJobTrigger.id,
- targetId: releaseJobTrigger.resourceId,
+ resourceId: releaseJobTrigger.resourceId,
releaseId: releaseJobTrigger.releaseId,
status: job.status,
createdAt: job.createdAt,
rank: sql<number>`ROW_NUMBER() OVER (
PARTITION BY ${releaseJobTrigger.resourceId}, ${releaseJobTrigger.releaseId}
ORDER BY ${job.createdAt} DESC
)`.as("rank"),
})Committable suggestion skipped: line range outside the PR's diff.
| v[0]!.environment_policy != null | ||
| ? rolloutDateFromReleaseJobTrigger( | ||
| v[0]!.release_job_trigger.targetId, | ||
| v[0]!.release_job_trigger.resourceId, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent Parameter Naming in Function Call
In the call to rolloutDateFromReleaseJobTrigger, the first argument is v[0]!.release_job_trigger.resourceId, but the function parameter is still named targetId. For consistency and clarity, consider renaming the function parameter from targetId to resourceId.
Apply this diff to update the parameter name:
-const rolloutDateFromReleaseJobTrigger = (
- targetId: string,
+const rolloutDateFromReleaseJobTrigger = (
+ resourceId: string,
releaseId: string,
environmentId: string,
releaseCreatedAt: Date,
environmentPolicyDuration: number,
releaseWindows: Array<{
startTime: Date;
endTime: Date;
recurrence: string;
}>,
) => {Committable suggestion skipped: line range outside the PR's diff.
| .query(({ ctx, input }) => | ||
| releaseJobTriggerQuery(ctx.db) | ||
| .where(eq(target.id, input)) | ||
| .where(eq(resource.id, input)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Procedure Name and Variable Inconsistency
The procedure byTargetId still uses Target in its name, but the query has been updated to use resource.id. For clarity and consistency, consider renaming the procedure to byResourceId and updating any related variable names from target to resource.
Apply this diff to rename the procedure and variables:
-export const jobRouter = createTRPCRouter({
- byTargetId: protectedProcedure
+export const jobRouter = createTRPCRouter({
+ byResourceId: protectedProcedure
.meta({
authorizationCheck: ({ canUser, input }) =>
- canUser.perform(Permission.SystemList).on({ type: "resource", id: input }),
+ canUser.perform(Permission.SystemList).on({ type: "resource", id: input }),
})
.input(z.string())
.query(({ ctx, input }) =>
releaseJobTriggerQuery(ctx.db)
.where(eq(resource.id, input))
.limit(1_000)
.orderBy(desc(job.createdAt), desc(releaseJobTrigger.createdAt))
.then((data) =>
data.map((t) => ({
...t.release_job_trigger,
job: t.job,
agent: t.job_agent,
- target: t.resource,
+ resource: t.resource,
deployment: t.deployment,
release: { ...t.release },
environment: t.environment,
})),
),
),
});| .from(resource) | ||
| .where(eq(resource.id, input.targetId)) | ||
| .then(takeFirstOrNull); | ||
| if (!t) throw new Error("Target not found"); |
There was a problem hiding this comment.
I think this is a miss.
| .where(eq(resource.id, input)) | ||
| .limit(1_000) | ||
| .orderBy(desc(job.createdAt), desc(releaseJobTrigger.createdAt)) | ||
| .then((data) => |
There was a problem hiding this comment.
I think this is a miss?
| value: deploymentVariableValue.value, | ||
| variableId: deploymentVariableValue.variableId, | ||
| targetFilter: deploymentVariableValue.targetFilter, | ||
| targetFilter: deploymentVariableValue.resourceFilter, |
There was a problem hiding this comment.
I think this is a miss.
| } | ||
|
|
||
| await db.delete(schema.target).where(eq(schema.target.id, target.id)); | ||
| await db.delete(schema.resource).where(eq(schema.resource.id, target.id)); |
| .perform(Permission.TargetMetadataGroupCreate) | ||
| .on({ type: "workspace", id: input.workspaceId }), | ||
| }) | ||
| .input(createTargetMetadataGroup) |
| .select({ count: count() }) | ||
| .from(schema.target) | ||
| .from(schema.resource) | ||
| .where(schema.targetMatchesMetadata(ctx.db, view.filter)) |
| schema.resource.workspaceId, | ||
| input.workspaceId, | ||
| ); | ||
| const targetConditions = schema.targetMatchesMetadata( |
| where: inArray(schema.resource.id, input), | ||
| }); | ||
| const events = ( | ||
| await Promise.allSettled(targets.map(getEventsForTargetDeleted)) |
| import { jobAgent } from "./job-agent.js"; | ||
| import { release } from "./release.js"; | ||
| import { target } from "./target.js"; | ||
| import { resource } from "./target.js"; |
There was a problem hiding this comment.
Let's rename to resource.js
| return eq(environment.id, cond.value); | ||
| if (cond.type === FilterType.Version) return buildVersionCondition(cond); | ||
| if (cond.type === JobFilterType.JobTarget) return eq(target.id, cond.value); | ||
| if (cond.type === JobFilterType.JobTarget) return eq(resource.id, cond.value); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
packages/api/src/router/target-provider.ts (6)
Line range hint
21-23: Update permission enums to match resource-centric modelThe permission checks still use target-centric terminology (
Permission.TargetList,Permission.TargetProviderUpdate) while the rest of the code has moved to resource-centric naming.Consider updating the permission enums in
@ctrlplane/validators/authto use resource terminology:- Permission.TargetList + Permission.ResourceList - Permission.TargetProviderUpdate + Permission.ResourceProviderUpdateAlso applies to: 89-92
Line range hint
70-82: Update response property names for consistencyThe response structure uses
targetCountwhich doesn't align with the resource-centric model.Consider updating the property name:
- targetCount: + resourceCount:
Line range hint
142-146: Update queue operation parametersThe queue operations still use target-centric terminology (
targetProviderId) while the rest of the code has moved to resource-centric naming.Consider updating the queue parameters:
- { targetProviderId: tg.id } + { resourceProviderId: tg.id }
Line range hint
153-161: Update input validation schema propertiesThe input validation schema still uses target-centric terminology (
targetProviderId).Consider updating the schema properties:
- targetProviderId: z.string().uuid(), + resourceProviderId: z.string().uuid(),
Line range hint
208-211: Update permission checks in delete mutationThe permission check still uses target-centric terminology (
Permission.TargetDelete).Consider updating the permission check:
- .perform(Permission.TargetDelete) + .perform(Permission.ResourceDelete)
Line range hint
216-217: Update input schema property namesThe delete mutation's input schema still uses target-centric terminology (
deleteTargets).Consider updating the property name:
- deleteTargets: z.boolean().optional().default(false), + deleteResources: z.boolean().optional().default(false),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
apps/webservice/next.config.js(0 hunks)packages/api/src/router/deployment-variable.ts(11 hunks)packages/api/src/router/target-provider.ts(7 hunks)
💤 Files with no reviewable changes (1)
- apps/webservice/next.config.js
🔇 Additional comments (8)
packages/api/src/router/target-provider.ts (1)
Line range hint 1-240: Verify complete transition to resource-centric terminology
Let's verify that all references to the target-centric model have been identified and updated.
packages/api/src/router/deployment-variable.ts (7)
24-24: LGTM: Import addition aligns with resource model transition
The addition of the resource import is consistent with the codebase's transition from a target-centric to a resource-centric model.
Line range hint 121-131: LGTM: Environment filter property renamed consistently
The renaming from targetFilter to resourceFilter in the environment query and conditions mapping is consistent with the resource model transition while preserving the existing logic.
179-181: LGTM: Property access updated to use resourceFilter
The property access has been correctly updated to use resourceFilter while maintaining the existing logic.
193-195: LGTM: Property access updated to use resourceFilter
The property access has been correctly updated to use resourceFilter while maintaining the existing logic.
216-220: LGTM: Query updated to use resource table
The queries have been correctly updated to use the resource table instead of the target table, maintaining the same logical flow.
Line range hint 363-381: LGTM: Subquery field renamed to resourceFilter
The subquery field has been correctly renamed from targetFilter to resourceFilter while maintaining the existing query structure and logic.
Line range hint 301-333: LGTM with verification: Table references updated to resource model
The table references have been correctly updated to use the resource table. The procedure name retention (byTargetId) appears intentional for API compatibility.
Let's verify that this endpoint is still being used with the old name:
✅ Verification successful
The search results confirm that the byTargetId endpoint is actively used across multiple frontend components, primarily in target-related views and drawers. The endpoint name consistency is important for maintaining these existing frontend integrations. Let me verify if there are any ongoing migrations or TODOs related to this naming:
LGTM: Table references correctly updated while maintaining API compatibility
The changes correctly update the table references from target to resource while intentionally preserving the byTargetId procedure name. This approach maintains compatibility with multiple frontend components that actively use this endpoint across target-related views and drawers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for API endpoint usage
rg "byTargetId" --type ts
Length of output: 1901
Script:
#!/bin/bash
# Search for any migration or TODO comments related to target/resource renaming
rg -i "todo.*target|target.*migration" --type ts
Length of output: 50
| .update(resourceProvider) | ||
| .set({ name: input.name }) | ||
| .where(eq(targetProvider.id, input.targetProviderId)) | ||
| .where(eq(resourceProvider.id, input.targetProviderId)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update variable references in update mutation
Some variable references in the update mutation still use the old terminology.
Consider updating these references:
- .where(eq(resourceProvider.id, input.targetProviderId))
+ .where(eq(resourceProvider.id, input.resourceProviderId))
- resourceProviderGoogle.resourceProviderId,
- input.targetProviderId,
+ resourceProviderGoogle.resourceProviderId,
+ input.resourceProviderId,Also applies to: 178-180
Summary by CodeRabbit