Skip to content

fix: Rename resource tables #215

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 13, 2024
Merged

fix: Rename resource tables #215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
981d031
fix: Rename resource tables
adityachoudhari26 Nov 13, 2024
9112f6f
Merge branch 'main' of github.com:sizzldev/ctrlplane into rename-reso…
adityachoudhari26 Nov 13, 2024
60f23e2
cleanup
adityachoudhari26 Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions apps/event-worker/src/target-scan/gke.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ export const getGkeTargets = async (
clusters.map((cluster) =>
clusterToTarget(
workspace.id,
config.targetProviderId,
config.resourceProviderId,
project,
cluster,
),
Expand Down Expand Up @@ -272,7 +272,7 @@ export const getGkeTargets = async (
project,
cluster,
workspace.id,
config.targetProviderId,
config.resourceProviderId,
)),
);

Expand All @@ -283,7 +283,7 @@ export const getGkeTargets = async (
project,
cluster,
workspace.id,
config.targetProviderId,
config.resourceProviderId,
)),
);

Expand Down
14 changes: 7 additions & 7 deletions apps/event-worker/src/target-scan/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ import { Queue, Worker } from "bullmq";
import { eq, takeFirstOrNull } from "@ctrlplane/db";
import { db } from "@ctrlplane/db/client";
import {
targetProvider,
targetProviderGoogle,
resourceProvider,
resourceProviderGoogle,
Comment on lines +9 to +10
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Incomplete renaming of target to resource detected

Multiple instances of target and Target remain in the codebase, indicating that the renaming process is not fully complete. Please update the following files to ensure consistency:

  • packages/api/src/dispatch.ts
  • apps/event-worker/src/target-scan/index.ts
  • packages/api/src/router/target-provider.ts
  • packages/validators/src/events/index.ts
  • packages/api/src/router/target.ts
  • apps/webservice/src/app/[workspaceSlug]/(targets)/target-providers/page.tsx
  • apps/webservice/src/app/[workspaceSlug]/(targets)/target-providers/add/page.tsx
  • apps/webservice/src/app/[workspaceSlug]/(targets)/target-providers/integrations/google/UpdateGoogleProviderDialog.tsx
  • apps/webservice/src/app/[workspaceSlug]/(targets)/layout.tsx
  • apps/event-worker/src/target-scan/gke.ts
🔗 Analysis chain

Verify consistency of resource/target terminology

While the schema imports have been updated to use resource terminology, other imports still use target (e.g., InsertTarget, TargetScanEvent). This mixing of terms could lead to confusion.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for remaining "target" references in schema and related files
rg -l "target" --type ts apps/event-worker/src/
rg -l "Target" --type ts apps/event-worker/src/

# Check if there are any remaining files using old schema references
rg -l "targetProvider" --type ts

Length of output: 7449

workspace,
} from "@ctrlplane/db/schema";
import { upsertTargets } from "@ctrlplane/job-dispatch";
Expand All @@ -31,12 +31,12 @@ export const createTargetScanWorker = () =>

const tp = await db
.select()
.from(targetProvider)
.where(eq(targetProvider.id, targetProviderId))
.innerJoin(workspace, eq(targetProvider.workspaceId, workspace.id))
.from(resourceProvider)
.where(eq(resourceProvider.id, targetProviderId))
.innerJoin(workspace, eq(resourceProvider.workspaceId, workspace.id))
.leftJoin(
targetProviderGoogle,
eq(targetProvider.id, targetProviderGoogle.targetProviderId),
resourceProviderGoogle,
eq(resourceProvider.id, resourceProviderGoogle.resourceProviderId),
Comment on lines +34 to +39
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Parameter name doesn't match updated schema

While the database query has been updated to use resourceProvider, the incoming parameter is still named targetProviderId. This inconsistency should be addressed.

- const { targetProviderId } = job.data;
+ const { resourceProviderId } = job.data;

  const tp = await db
    .select()
    .from(resourceProvider)
-   .where(eq(resourceProvider.id, targetProviderId))
+   .where(eq(resourceProvider.id, resourceProviderId))

Committable suggestion skipped: line range outside the PR's diff.

)
.then(takeFirstOrNull);

Expand Down
12 changes: 0 additions & 12 deletions apps/webservice/next.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,18 +33,6 @@ const config = {
],
},

async rewrites() {
return [
{
source: "/api/v1/target/proxy/controller",
destination: "http://localhost:4000/api/v1/target/proxy/controller",
},
{
source: "/api/v1/target/proxy/session/:path*",
destination: "http://localhost:4000/api/v1/target/proxy/session/:path*",
},
];
},
/** We already do linting and typechecking as separate tasks in CI */
eslint: { ignoreDuringBuilds: true },
typescript: { ignoreBuildErrors: true },
Expand Down
7 changes: 2 additions & 5 deletions apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/Filter.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ export const EditFilterForm: React.FC<{
const update = api.environment.update.useMutation();
const form = useForm({
schema: filterForm,
defaultValues: { targetFilter: getFilter(environment.targetFilter) },
defaultValues: { targetFilter: getFilter(environment.resourceFilter) },
});

const { targetFilter } = form.watch();
Expand All @@ -141,10 +141,7 @@ export const EditFilterForm: React.FC<{
update
.mutateAsync({
id: environment.id,
data: {
...data,
targetFilter: targetFilter ?? null,
},
data: { ...data, resourceFilter: targetFilter ?? null },
})
.then(() => form.reset(data))
.then(() => utils.environment.bySystemId.invalidate(environment.systemId))
Expand Down
3 changes: 2 additions & 1 deletion ...webservice/src/app/[workspaceSlug]/_components/job-condition/JobTargetConditionRender.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@ export const JobTargetConditionRender: React.FC<
);
const system = systemQ.data;
const envFilters =
system?.environments.map((env) => env.targetFilter).filter(isPresent) ?? [];
system?.environments.map((env) => env.resourceFilter).filter(isPresent) ??
[];
Comment on lines +62 to +63
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Incomplete migration: targetFilter still used in multiple files.

The migration from targetFilter to resourceFilter is incomplete. The following files still reference targetFilter and need to be updated:

  • packages/node-sdk/src/schema.ts
  • packages/job-dispatch/src/job-variables-deployment/utils.ts
  • apps/webservice/src/app/api/v1/environments/openapi.ts
  • apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/environments/TargetFilterUniquenessIndicator.tsx
  • apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/variables/VariableValueDropdown.tsx
  • apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/Filter.tsx
  • packages/api/src/router/deployment-variable.ts
  • apps/webservice/src/app/[workspaceSlug]/systems/SystemActionsDropdown.tsx
  • packages/events/src/triggers/target-deleted.ts
  • packages/events/src/triggers/deployment-deleted.ts
  • packages/events/src/triggers/environment-deleted.ts
  • packages/job-dispatch/src/target.ts
  • packages/job-dispatch/src/environment-creation.ts
  • packages/db/src/schema/environment.ts
  • packages/db/src/schema/deployment-variables.ts
  • packages/job-dispatch/src/release-job-trigger.ts
  • packages/job-dispatch/src/job-variables-deployment/utils.ts
  • packages/job-dispatch/src/job-variables-deployment/job-variables-deployment.ts
  • packages/api/src/router/deployment.ts
  • packages/api/src/router/deployment-variable.ts
  • apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/page.tsx
  • apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/variables/page.tsx
  • packages/job-dispatch/src/__test__/job-variables-deployment.test.ts
  • apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/variables/VariableValueDropdown.tsx
  • apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/_components/variables/VariableInputs.tsx
  • packages/api/src/router/environment.ts
  • apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/Filter.tsx
  • apps/webservice/src/app/[workspaceSlug]/_components/job-condition/JobTargetConditionRender.tsx
🔗 Analysis chain

LGTM! Verify environment filter migration.

The change from targetFilter to resourceFilter aligns with the PR objective. However, let's verify that all environment configurations have been migrated.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all environment configurations use resourceFilter
# and none are still using targetFilter

# Search for any remaining targetFilter references in environment configurations
rg -l "targetFilter" --type ts --type tsx

# Search for new resourceFilter references to confirm migration
rg -l "resourceFilter" --type ts --type tsx

Length of output: 141

Script:

#!/bin/bash
# Description: Verify that all environment configurations use resourceFilter
# and none are still using targetFilter

# Search for any remaining targetFilter references in environment configurations
rg -l "targetFilter" --glob "*.ts*" 

# Search for new resourceFilter references to confirm migration
rg -l "resourceFilter" --glob "*.ts*"

Length of output: 1973


const systemFilter: TargetCondition = {
type: FilterType.Comparison,
Expand Down
2 changes: 1 addition & 1 deletion ...bservice/src/app/[workspaceSlug]/_components/target-drawer/CreateTargetVariableDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ export const CreateTargetVariableDialog: React.FC<
const utils = api.useUtils();
const onSubmit = form.handleSubmit((data) =>
createTargetVariable
.mutateAsync({ targetId, ...data })
.mutateAsync({ resourceId: targetId, ...data })
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

LGTM, but parameter naming is inconsistent with component interface.

The change from targetId to resourceId in the mutation call is correct as part of the resource-centric model transition. However, this creates an inconsistency where the prop is named targetId but used as resourceId internally.

.then(() => utils.target.byId.invalidate(targetId))
.then(() => form.reset())
.then(() => setOpen(false)),
Expand Down
2 changes: 1 addition & 1 deletion ...webservice/src/app/[workspaceSlug]/_components/target-drawer/EditTargetVariableDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ export const EditTargetVariableDialog: React.FC<
updateTargetVariable
.mutateAsync({ id: targetVariable.id, data })
.then(() => form.reset(data))
.then(() => utils.target.byId.invalidate(targetVariable.targetId))
.then(() => utils.target.byId.invalidate(targetVariable.resourceId))
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Multiple remaining references to targetId found; refactoring appears incomplete.

🔗 Analysis chain

Verify schema and API endpoint alignment with the resource-centric model.

The change from targetId to resourceId aligns with the PR's objective of transitioning to a resource-centric model. However, we should verify the related changes.

Let's verify the schema and API endpoint alignment:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the schema changes and API endpoint alignment

# Check if the schema has been updated to use resourceId
ast-grep --pattern 'type TargetVariable = {
  $$$
  resourceId: $_
  $$$
}'

# Check if there are any remaining references to targetId in the codebase
rg 'targetId.*=.*targetVariable\.' --type ts

Length of output: 264

Script:

#!/bin/bash
# Description: Inspect the context of targetId usage in TargetVariableDropdown.tsx and search for all remaining targetId references.

# Extract lines surrounding the targetId usage for context
rg 'targetId\s*=' apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/TargetVariableDropdown.tsx -A 3 -B 3

# Search for all remaining references to targetId in the codebase
rg 'targetId' --type ts

Length of output: 23027

.then(() => setOpen(false)),
);

Expand Down
2 changes: 1 addition & 1 deletion apps/webservice/src/app/[workspaceSlug]/_components/target-drawer/TargetVariableDropdown.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export const TargetVariableDropdown: React.FC<TargetVariableDropdownProps> = ({
</EditTargetVariableDialog>
<DeleteTargetVariableDialog
variableId={targetVariable.id}
targetId={targetVariable.targetId}
targetId={targetVariable.resourceId}
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

DeleteTargetVariableDialog still expects targetId. Please update it to handle resourceId.

🔗 Analysis chain

Verify related component updates

The prop value has been updated to use resourceId, but we should verify that the receiving component (DeleteTargetVariableDialog) has been similarly updated to handle this change.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if DeleteTargetVariableDialog has been updated to handle resourceId

# Look for the DeleteTargetVariableDialog component definition
ast-grep --pattern 'type $_ = {
  $$$
  targetId: $_
  $$$
}'

# Check for any remaining references to targetId in related files
rg -g '*.{ts,tsx}' 'targetId.*resourceId' ./apps/webservice/src/app

Length of output: 3978

onClose={() => setOpen(false)}
>
<DropdownMenuItem
Expand Down
2 changes: 1 addition & 1 deletion apps/webservice/src/app/[workspaceSlug]/systems/SystemActionsDropdown.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export const SystemActionsDropdown: React.FC<SystemActionsDropdownProps> = ({
}) => {
const { workspaceSlug } = useParams<{ workspaceSlug: string }>();
const envFilters = system.environments
.map((env) => env.targetFilter)
.map((env) => env.resourceFilter)
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Incomplete rename of targetFilter detected. Please update all instances to resourceFilter to ensure consistency.

  • openapi.v1.json
  • packages/node-sdk/src/schema.ts
  • packages/job-dispatch/src/job-variables-deployment/utils.ts
  • packages/api/src/router/deployment-variable.ts
  • apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/environments/TargetFilterUniquenessIndicator.tsx
  • apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/variables/VariableValueDropdown.tsx
  • apps/webservice/src/app/[workspaceSlug]/_components/environment-drawer/Filter.tsx
  • apps/webservice/src/app/api/v1/environments/openapi.ts
🔗 Analysis chain

LGTM! Consistent with resource-centric model transition.

The change from targetFilter to resourceFilter aligns with the PR's objective of transitioning to a resource-centric model.

Let's verify this change is consistent across the codebase:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining instances of targetFilter that might need updating
# and verify resourceFilter is used consistently

# Check for any remaining targetFilter instances
echo "Checking for remaining targetFilter instances:"
rg "targetFilter" -l

# Check for consistent usage of resourceFilter
echo "Verifying resourceFilter usage:"
rg "resourceFilter" -l

# Check specifically in environment-related files
echo "Checking environment-related files:"
ast-grep --pattern 'type Environment = {
  $$$
}'

Length of output: 2268

.filter(isPresent);
const filter: TargetCondition = {
type: TargetFilterType.Comparison,
Expand Down
4 changes: 2 additions & 2 deletions ...ice/src/app/[workspaceSlug]/systems/[systemSlug]/_components/variables/VariableInputs.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,8 @@ export const VariableTargetInput: React.FC<
});
const envs = envsQ.data ?? [];
const envConditions = envs
.filter((e) => e.targetFilter != null)
.map((e) => e.targetFilter!);
.filter((e) => e.resourceFilter != null)
.map((e) => e.resourceFilter!);

const tFilterConditions: TargetCondition[] = [
{
Expand Down
2 changes: 1 addition & 1 deletion .../webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/TableDeployments.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ const ReleaseCell: React.FC<{
(releaseJobTrigger) =>
isPresent(releaseJobTrigger.environmentId) &&
isPresent(releaseJobTrigger.releaseId) &&
isPresent(releaseJobTrigger.targetId),
isPresent(releaseJobTrigger.target.id),
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Inconsistent Usage of targetId Detected

Multiple instances of targetId remain in the codebase. Ensure all references are updated to target.id to maintain consistency.

🔗 Analysis chain

Verify data structure changes across the codebase

The change from releaseJobTrigger.targetId to releaseJobTrigger.target.id suggests a shift in the data structure. Ensure that all consumers of this data are updated to handle the new nested object structure.

Let's verify the consistency of this change across the codebase:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining direct targetId references that might need updating

# Search for direct targetId references
rg "targetId" -A 5

# Search for the new target.id pattern
rg "target\.id" -A 5

Length of output: 124

Script:

#!/bin/bash
# Description: Check for any remaining direct targetId references that might need updating

# Search for direct targetId references in .ts and .tsx files
rg "targetId" --glob "*.ts" --glob "*.tsx" -A 5

# Search for the new target.id pattern in .ts and .tsx files
rg "target\.id" --glob "*.ts" --glob "*.tsx" -A 5

Length of output: 124110

)
.map((releaseJobTrigger) => ({ ...releaseJobTrigger }));
return (
Expand Down
10 changes: 5 additions & 5 deletions ...ervice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ const Variables: React.FC<{
operator: TargetOperator.Or,
conditions: await api.environment
.bySystemId(deployment.systemId)
.then((envs) => envs.map((e) => e.targetFilter).filter(isPresent)),
.then((envs) => envs.map((e) => e.resourceFilter).filter(isPresent)),
};

const variablesPromises = variablesByDeployment.map(async (variable) => {
Expand All @@ -43,7 +43,7 @@ const Variables: React.FC<{
const rest = variable.values.filter((v) => v.id !== defaultValue?.id);

const valuesPromises = rest.map(async (v) => {
if (v.targetFilter == null)
if (v.resourceFilter == null)
return {
...v,
targetCount: 0,
Expand All @@ -52,13 +52,13 @@ const Variables: React.FC<{
};

const filterHash = LZString.compressToEncodedURIComponent(
JSON.stringify(v.targetFilter),
JSON.stringify(v.resourceFilter),
);

const filter: ComparisonCondition = {
type: TargetFilterType.Comparison,
operator: TargetOperator.And,
conditions: [systemTargetsFilter, v.targetFilter],
conditions: [systemTargetsFilter, v.resourceFilter],
};

const targets = await api.target.byWorkspaceId.list({
Expand All @@ -78,7 +78,7 @@ const Variables: React.FC<{
const values = await Promise.all(valuesPromises);

if (defaultValue != null) {
const restFilters = rest.map((v) => v.targetFilter).filter(isPresent);
const restFilters = rest.map((v) => v.resourceFilter).filter(isPresent);
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Update target-centric names for consistency.

Numerous instances of target-centric terminology were found across the codebase, including:

  • Variable names (e.g., targets, targetCount)
  • API endpoints (e.g., api.target.byWorkspaceId)
  • Types (e.g., TargetCondition)

Refactoring these for consistency with resource-centric naming will enhance maintainability.

🔗 Analysis chain

Consider updating related target-centric names for consistency.

While restFilters now correctly uses resourceFilter, there are several related names that still use the target-centric terminology:

  • Variable names like targets, targetCount
  • API endpoint api.target.byWorkspaceId
  • Type TargetCondition

This mix of resource-centric and target-centric terminology could be confusing for future maintenance.

Let's check for other occurrences of target-centric naming that might need updates:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for target-related API endpoints and types
rg "api\.target\." --type typescript
rg "type.*Target.*=" --type typescript

Length of output: 87

Script:

#!/bin/bash
# Comprehensive search for any target-related terms in TypeScript files
rg "target" --type typescript

Length of output: 64

Script:

#!/bin/bash
# Search for target-related terms in TypeScript files using correct file extensions
rg "target" --glob "*.ts" --glob "*.tsx"

Length of output: 177455


const filter: TargetCondition =
restFilters.length === 0
Expand Down
2 changes: 1 addition & 1 deletion ...ems/[systemSlug]/deployments/[deploymentSlug]/releases/[versionId]/TargetReleaseTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,7 @@ export const TargetReleaseTable: React.FC<TargetReleaseTableProps> = ({
environment: environments.find(
(e) => e.id === triggers[0]!.environmentId,
),
targets: _.groupBy(triggers, (t) => t.targetId),
targets: _.groupBy(triggers, (t) => t.target.id),
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Additional groupBy(target.id) instance found

An additional instance of grouping by target.id was found in apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/deployments/TableCells.tsx. Please review this in the context of the resource-centric model transition.

🔗 Analysis chain

Verify the impact of grouping change

The grouping by t.target.id appears unchanged in behavior but should be reviewed in the context of the resource-centric model transition.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for other instances of target.id grouping
# to ensure consistent transition to resource-centric model

# Search for similar grouping patterns
rg -U "groupBy.*target.id|target.id.*groupBy" --type tsx --type ts

# Search for other potential target references that might need updating
rg -l "target\." --type tsx --type ts

Length of output: 164

Script:

#!/bin/bash
# Description: Check for other instances of target.id grouping
# to ensure consistent transition to resource-centric model

# Search for similar grouping patterns in .tsx and .ts files
rg -U "groupBy.*target\.id|target\.id.*groupBy" -g "*.tsx" -g "*.ts"

# Search for other potential target references that might need updating in .tsx and .ts files
rg -l "target\." -g "*.tsx" -g "*.ts"

Length of output: 8470

}))
.filter((t) => isPresent(t.environment))
.value();
Expand Down
4 changes: 2 additions & 2 deletions ...ug]/systems/[systemSlug]/deployments/[deploymentSlug]/variables/VariableValueDropdown.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ const EditVariableValueDialog: React.FC<{
schema: editVariableValueFormSchema,
defaultValues: {
value: value.value,
targetFilter: value.targetFilter,
targetFilter: value.resourceFilter,
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Complete the target-to-resource renaming transition

While the data layer has been updated to use resourceFilter, the UI layer (form schema, field names, etc.) still uses targetFilter. This inconsistency could lead to confusion and maintenance issues.

Consider applying this comprehensive rename:

-const editVariableValueFormSchema = z.object({
-  value: z.union([z.string(), z.number(), z.boolean()]),
-  targetFilter: targetCondition
+const editVariableValueFormSchema = z.object({
+  value: z.union([z.string(), z.number(), z.boolean()]),
+  resourceFilter: targetCondition // Note: Consider renaming targetCondition to resourceCondition as well
     .nullish()
     .refine((data) => data == null || isValidTargetCondition(data), {
       message: "Invalid target condition",
     }),
   default: z.boolean().optional(),
 });

 const form = useForm({
   schema: editVariableValueFormSchema,
   defaultValues: {
     value: value.value,
-    targetFilter: value.resourceFilter,
+    resourceFilter: value.resourceFilter,
     default: variable.defaultValueId === value.id,
   },
 });

 const onSubmit = form.handleSubmit((data) =>
   update
     .mutateAsync({
       id: value.id,
-      data: { ...data, resourceFilter: data.targetFilter },
+      data: data,
     })

Also update the form field:

 <FormField
   control={form.control}
-  name="targetFilter"
+  name="resourceFilter"
   render={({ field: { value, onChange } }) => (
     <FormItem>
-      <FormLabel>Target filter</FormLabel>
+      <FormLabel>Resource filter</FormLabel>

And the clear filter button:

 <Button
   variant="outline"
   type="button"
-  onClick={() => form.setValue("targetFilter", null)}
+  onClick={() => form.setValue("resourceFilter", null)}
 >
   Clear filter
 </Button>

Also applies to: 92-92

default: variable.defaultValueId === value.id,
},
});
Expand All @@ -89,7 +89,7 @@ const EditVariableValueDialog: React.FC<{
update
.mutateAsync({
id: value.id,
data,
data: { ...data, resourceFilter: data.targetFilter },
})
.then(() => router.refresh())
.then(onClose),
Expand Down
10 changes: 5 additions & 5 deletions .../app/[workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/variables/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export default async function VariablesPage({
operator: TargetOperator.Or,
conditions: await api.environment
.bySystemId(deployment.systemId)
.then((envs) => envs.map((e) => e.targetFilter).filter(isPresent)),
.then((envs) => envs.map((e) => e.resourceFilter).filter(isPresent)),
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Incomplete Transition to Resource-Centric Terminology Detected

The transition from target-centric to resource-centric terminology is not fully implemented. Numerous files still use target-specific types and operators.

Areas Needing Attention:

  • packages/validators/src/targets/conditions/
  • packages/jobs/src/conditions/
  • packages/db/src/schema/
  • apps/webservice/src/app/[workspaceSlug]/systems/[systemSlug]/
🔗 Analysis chain

Verify complete transition from target to resource terminology

While targetFilter has been renamed to resourceFilter, the surrounding code still uses target-specific types and operators (e.g., TargetFilterType, TargetOperator). This mixed terminology could lead to confusion.

Consider creating a follow-up task to rename these types to maintain consistency with the resource-centric model:

  • TargetFilterType → ResourceFilterType
  • TargetOperator → ResourceOperator
  • TargetCondition → ResourceCondition
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for remaining target-related types that should be renamed
rg "Target(Filter|Operator|Condition)" --type ts

Length of output: 35420

};

const variablesPromises = variablesByDeployment.map(async (variable) => {
Expand All @@ -41,7 +41,7 @@ export default async function VariablesPage({
const rest = variable.values.filter((v) => v.id !== defaultValue?.id);

const valuesPromises = rest.map(async (v) => {
if (v.targetFilter == null)
if (v.resourceFilter == null)
return {
...v,
targetCount: 0,
Expand All @@ -50,13 +50,13 @@ export default async function VariablesPage({
};

const filterHash = LZString.compressToEncodedURIComponent(
JSON.stringify(v.targetFilter),
JSON.stringify(v.resourceFilter),
);

const filter: ComparisonCondition = {
type: TargetFilterType.Comparison,
operator: TargetOperator.And,
conditions: [systemTargetsFilter, v.targetFilter],
conditions: [systemTargetsFilter, v.resourceFilter],
};

const targets = await api.target.byWorkspaceId.list({
Expand All @@ -76,7 +76,7 @@ export default async function VariablesPage({
const values = await Promise.all(valuesPromises);

if (defaultValue != null) {
const restFilters = rest.map((v) => v.targetFilter).filter(isPresent);
const restFilters = rest.map((v) => v.resourceFilter).filter(isPresent);

const filter: TargetCondition =
restFilters.length === 0
Expand Down
4 changes: 2 additions & 2 deletions apps/webservice/src/app/api/v1/job-agents/[agentId]/jobs/running/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
job,
release,
releaseJobTrigger,
target,
resource,
} from "@ctrlplane/db/schema";

import { getUser } from "~/app/api/v1/auth";
Expand All @@ -27,7 +27,7 @@ export const GET = async (
.from(job)
.innerJoin(releaseJobTrigger, eq(releaseJobTrigger.jobId, job.id))
.leftJoin(environment, eq(environment.id, releaseJobTrigger.environmentId))
.leftJoin(target, eq(target.id, releaseJobTrigger.targetId))
.leftJoin(resource, eq(resource.id, releaseJobTrigger.resourceId))
.leftJoin(release, eq(release.id, releaseJobTrigger.releaseId))
.leftJoin(deployment, eq(deployment.id, release.deploymentId))
.where(
Expand Down
10 changes: 5 additions & 5 deletions apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@ import {
jobVariable,
release,
releaseJobTrigger,
resource,
resourceMetadata,
runbook,
runbookJobTrigger,
target,
targetMetadata,
updateJob,
user,
} from "@ctrlplane/db/schema";
Expand Down Expand Up @@ -80,7 +80,7 @@ export const GET = request()
environment,
eq(environment.id, releaseJobTrigger.environmentId),
)
.leftJoin(target, eq(target.id, releaseJobTrigger.targetId))
.leftJoin(resource, eq(resource.id, releaseJobTrigger.resourceId))
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider updating response mapping to use consistent terminology.

While the database query correctly uses the new resource table, the response still uses target as the key when returning the resource data. This inconsistency could be confusing for API consumers.

Consider updating the response mapping to use consistent terminology:

      .then((row) => ({
        job: row.job,
        runbook: row.runbook,
        environment: row.environment,
-       target: row.resource,
+       resource: row.resource,
        deployment: row.deployment,
        release: row.release,
      }));

Note: This change might require coordinated updates in API clients and documentation.

Also applies to: 89-94

.leftJoin(release, eq(release.id, releaseJobTrigger.releaseId))
.leftJoin(deployment, eq(deployment.id, release.deploymentId))
.where(eq(job.id, params.jobId))
Expand Down Expand Up @@ -116,8 +116,8 @@ export const GET = request()

const jobTargetMetadataRows = await db
.select()
.from(targetMetadata)
.where(eq(targetMetadata.targetId, je.target?.id ?? ""));
.from(resourceMetadata)
.where(eq(resourceMetadata.resourceId, je.target?.id ?? ""));

const metadata = Object.fromEntries(
jobTargetMetadataRows.map((m) => [m.key, m.value]),
Expand Down
12 changes: 8 additions & 4 deletions apps/webservice/src/app/api/v1/target-providers/[providerId]/set/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@ import { z } from "zod";

import { eq, takeFirstOrNull } from "@ctrlplane/db";
import { db } from "@ctrlplane/db/client";
import { createTarget, targetProvider, workspace } from "@ctrlplane/db/schema";
import {
createTarget,
resourceProvider,
workspace,
} from "@ctrlplane/db/schema";
Comment on lines +7 to +11
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Inconsistent naming: 'createTarget' still present

Multiple files still reference "createTarget" or "targetProvider", indicating that the refactoring to resource-centric terminology is incomplete.

  • apps/webservice/src/app/api/v1/targets/route.ts
  • packages/db/src/schema/target.ts
  • packages/db/src/schema/target-group.ts
  • apps/webservice/src/app/[workspaceSlug]/(targets)/target-providers/page.tsx
  • apps/webservice/src/app/[workspaceSlug]/_components/CreateTarget.tsx
🔗 Analysis chain

Verify naming consistency across schema imports

While targetProvider has been renamed to resourceProvider, createTarget still uses the old naming convention. This might indicate incomplete refactoring.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining "target" references in schema definitions
# and their corresponding usage in the codebase

# Check schema definitions
echo "Checking schema definitions..."
rg -l "createTarget|targetProvider" --type ts

# Check for mixed usage of target/resource terminology
echo "Checking for mixed terminology..."
ast-grep --pattern 'import { $$$ resource$$$ $$$ } from "@ctrlplane/db/schema"'

Length of output: 1428

import { upsertTargets } from "@ctrlplane/job-dispatch";
import { Permission } from "@ctrlplane/validators/auth";

Expand Down Expand Up @@ -55,9 +59,9 @@ export const PATCH = request()

const query = await db
.select()
.from(targetProvider)
.innerJoin(workspace, eq(workspace.id, targetProvider.workspaceId))
.where(eq(targetProvider.id, params.providerId))
.from(resourceProvider)
.innerJoin(workspace, eq(workspace.id, resourceProvider.workspaceId))
.where(eq(resourceProvider.id, params.providerId))
.then(takeFirstOrNull);

const provider = query?.resource_provider;
Expand Down
16 changes: 9 additions & 7 deletions apps/webservice/src/app/api/v1/targets/[targetId]/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ export const GET = request()
}),
)
.handle(async ({ db }, { params }: { params: { targetId: string } }) => {
const data = await db.query.target.findFirst({
where: eq(schema.target.id, params.targetId),
const data = await db.query.resource.findFirst({
where: eq(schema.resource.id, params.targetId),
Comment on lines +24 to +25
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Maintain consistency in resource terminology

While the database schema has been updated to use resource, the response structure and error messages still use "target" terminology. This inconsistency could lead to confusion in the API contract.

Consider updating the error message and variable names:

    if (data == null)
-     return NextResponse.json({ error: "Target not found" }, { status: 404 });
+     return NextResponse.json({ error: "Resource not found" }, { status: 404 });

-   const { metadata, ...target } = data;
+   const { metadata, ...resource } = data;

Committable suggestion skipped: line range outside the PR's diff.

with: {
metadata: true,
variables: true,
Expand Down Expand Up @@ -73,8 +73,8 @@ export const PATCH = request()
{ body: z.infer<typeof patchSchema> },
{ params: { targetId: string } }
>(async ({ db, body }, { params }) => {
const target = await db.query.target.findFirst({
where: eq(schema.target.id, params.targetId),
const target = await db.query.resource.findFirst({
where: eq(schema.resource.id, params.targetId),
Comment on lines +76 to +77
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Update PATCH handler to use resource terminology

The schema validation and variable names should be updated to maintain consistency with the resource model.

Consider renaming the schema and variables:

- const patchSchema = z.object({
+ const resourcePatchSchema = z.object({
  // ... schema definition ...
});

- const target = await db.query.resource.findFirst({
+ const resource = await db.query.resource.findFirst({
  where: eq(schema.resource.id, params.targetId),
});

if (target == null)
-  return NextResponse.json({ error: "Target not found" }, { status: 404 });
+  return NextResponse.json({ error: "Resource not found" }, { status: 404 });

Committable suggestion skipped: line range outside the PR's diff.

});

if (target == null)
Expand All @@ -95,14 +95,16 @@ export const DELETE = request()
),
)
.handle(async ({ db }, { params }: { params: { targetId: string } }) => {
const target = await db.query.target.findFirst({
where: eq(schema.target.id, params.targetId),
const target = await db.query.resource.findFirst({
where: eq(schema.resource.id, params.targetId),
Comment on lines +98 to +99
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Align DELETE handler with resource terminology

While the database operations have been updated, the variable names and error messages still use the old terminology.

Consider updating the variable names and error messages:

- const target = await db.query.resource.findFirst({
+ const resource = await db.query.resource.findFirst({
  where: eq(schema.resource.id, params.targetId),
});

- if (target == null)
-   return NextResponse.json({ error: "Target not found" }, { status: 404 });
+ if (resource == null)
+   return NextResponse.json({ error: "Resource not found" }, { status: 404 });

Also applies to: 105-107

});

if (target == null)
return NextResponse.json({ error: "Target not found" }, { status: 404 });

await db.delete(schema.target).where(eq(schema.target.id, params.targetId));
await db
.delete(schema.resource)
.where(eq(schema.resource.id, params.targetId));

return NextResponse.json({ success: true });
});
Loading
Loading