Skip to content
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

Jump to bottom

fix: Init refactor target -> resource #213

Merged
merged 4 commits into from
Nov 13, 2024
Merged

fix: Init refactor target -> resource #213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8da1cc1
init refactor
adityachoudhari26 Nov 12, 2024
972785d
more fix
adityachoudhari26 Nov 12, 2024
8051c7f
Merge branch 'main' of github.com:sizzldev/ctrlplane into init-rename…
adityachoudhari26 Nov 13, 2024
657af0d
rename enum
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
12 changes: 6 additions & 6 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 @@ -47,17 +47,17 @@ export const createTargetScanWorker = () =>
}

logger.info(
`Received scanning request for "${tp.target_provider.name}" (${targetProviderId}).`,
`Received scanning request for "${tp.resource_provider.name}" (${targetProviderId}).`,
);

const targets: InsertTarget[] = [];

if (tp.target_provider_google != null) {
if (tp.resource_provider_google != null) {
logger.info("Found Google config, scanning for GKE targets");
try {
const gkeTargets = await getGkeTargets(
tp.workspace,
tp.target_provider_google,
tp.resource_provider_google,
);
targets.push(...gkeTargets);
} catch (error: any) {
Expand All @@ -69,18 +69,18 @@ export const createTargetScanWorker = () =>

try {
logger.info(
`Upserting ${targets.length} targets for provider ${tp.target_provider.id}`,
`Upserting ${targets.length} targets for provider ${tp.resource_provider.id}`,
);
if (targets.length > 0) {
await upsertTargets(db, targets);
} else {
logger.info(
`No targets found for provider ${tp.target_provider.id}, skipping upsert.`,
`No targets found for provider ${tp.resource_provider.id}, skipping upsert.`,
);
}
} catch (error: any) {
logger.error(
`Error upserting targets for provider ${tp.target_provider.id}: ${error.message}`,
`Error upserting targets for provider ${tp.resource_provider.id}: ${error.message}`,
{ error },
);
}
Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -47,7 +47,7 @@ export const GET = async (
...row.job,
config: row.release_job_trigger,
environment: row.environment,
target: row.target,
target: row.resource,
deployment: row.deployment,
release: row.release,
})),
Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -89,7 +89,7 @@ export const GET = request()
job: row.job,
runbook: row.runbook,
environment: row.environment,
target: row.target,
target: row.resource,
deployment: row.deployment,
release: row.release,
}));
Expand Down
4 changes: 2 additions & 2 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 @@ -44,7 +44,7 @@ export const PATCH = request()
authz(({ can, extra }) =>
can
.perform(Permission.TargetUpdate)
.on({ type: "targetProvider", id: extra.params.providerId }),
.on({ type: "resourceProvider", id: extra.params.providerId }),
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Inconsistent authorization type with Permission enum

The authorization type has been changed to "resourceProvider" but this is inconsistent with the Permission enum which still uses "target" prefixes. The code is using Permission.TargetUpdate but checking against type: "resourceProvider", creating a mismatch in the authorization system.

  • packages/validators/src/auth/index.ts: Permission enum still uses TargetUpdate, TargetProviderUpdate, etc.
  • apps/webservice/src/app/api/v1/target-providers/[providerId]/set/route.ts: Uses Permission.TargetUpdate with type: "resourceProvider"

This inconsistency could lead to authorization failures and needs to be addressed by either:

  1. Reverting the authorization type back to "targetProvider" to match the Permission enum, or
  2. Updating the entire Permission enum to use "resource" terminology consistently
🔗 Analysis chain

Verify permission enum consistency

The authorization type has been changed to "resourceProvider", but we need to verify if this matches the Permission enum which still uses TargetUpdate.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if Permission enum has been updated consistently
# Look for Permission enum definition and its usage
ast-grep --pattern 'enum Permission {
  $$$
  TargetUpdate
  $$$
}'

# Check for any remaining "target" references in auth-related files
rg -g "**/{auth,permission}*" -i "target.*update"

Length of output: 121

Script:

#!/bin/bash
# Let's try a broader search for Permission enum and authorization types
# Search for Permission enum
ast-grep --pattern 'enum Permission {
  $$$
}'

# Search for authorization type definitions and usage
rg -g "!{*.lock,*.md}" "type: ['\"]resourceProvider['\"]|type: ['\"]targetProvider['\"]" -A 2 -B 2

# Search for permission-related imports and usage
rg -g "!{*.lock,*.md}" "Permission.*Target|Target.*Permission" -A 2 -B 2

Length of output: 22718

),
)
.handle<
Expand All @@ -60,7 +60,7 @@ export const PATCH = request()
.where(eq(targetProvider.id, params.providerId))
.then(takeFirstOrNull);

const provider = query?.target_provider;
const provider = query?.resource_provider;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Inconsistent terminology between schema and variable names

The variable has been renamed to resource_provider but it's being destructured from query?.resource_provider while the schema and table name is still targetProvider. This creates confusion and makes the code harder to maintain.

Either:

  1. Revert the variable name change until the schema is updated:
-const provider = query?.resource_provider;
+const provider = query?.target_provider;
  1. Or complete the schema rename first and update all related references.

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

if (!provider)
return NextResponse.json(
{ error: "Provider not found" },
Expand Down
6 changes: 3 additions & 3 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 @@ -17,7 +17,7 @@ export const GET = request()
authz(({ can, extra }) => {
return can
.perform(Permission.TargetGet)
.on({ type: "target", id: extra.params.targetId });
.on({ type: "resource", id: extra.params.targetId });
}),
)
.handle(async ({ db }, { params }: { params: { targetId: string } }) => {
Expand Down Expand Up @@ -65,7 +65,7 @@ export const PATCH = request()
authz(({ can, extra }) =>
can
.perform(Permission.TargetUpdate)
.on({ type: "target", id: extra.params.targetId }),
.on({ type: "resource", id: extra.params.targetId }),
),
)
.use(parseBody(patchSchema))
Expand All @@ -91,7 +91,7 @@ export const DELETE = request()
authz(({ can, extra }) =>
can
.perform(Permission.TargetDelete)
.on({ type: "target", id: extra.params.targetId }),
.on({ type: "resource", id: extra.params.targetId }),
),
)
.handle(async ({ db }, { params }: { params: { targetId: string } }) => {
Expand Down
4 changes: 2 additions & 2 deletions ...bservice/src/app/api/v1/workspaces/[workspaceId]/targets/identifier/[identifier]/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export const GET = request()
if (target == null) return false;
return can
.perform(Permission.TargetGet)
.on({ type: "target", id: target.id });
.on({ type: "resource", id: target.id });
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Permission naming inconsistency detected across the codebase

The verification reveals that while authorization types have been updated from "target" to "resource", the Permission enum still extensively uses "Target" prefixes across the codebase. Found inconsistencies:

  • Permission names still use "Target" prefix (e.g., TargetGet, TargetList, TargetUpdate, etc.)
  • These permissions are used with type: "resource" in authorization checks
  • Similar pattern found in multiple files:
    • packages/api/src/router/target.ts
    • packages/api/src/router/target-provider.ts
    • packages/api/src/router/target-metadata-group.ts
    • Various API route files
🔗 Analysis chain

Verify permission naming consistency

While the authorization type has been updated to "resource", the permission enum still uses TargetGet. This might indicate inconsistent terminology.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if Permission enum still uses "Target" prefix across the codebase
# and if there are any pending renames needed for consistency

# Search for Permission enum definition and usages
ast-grep --pattern 'enum Permission {
  $$$
}'

# Search for any remaining "Target" prefixed permissions
rg "Permission\.Target" -A 1

Length of output: 9241

}),
)
.handle<unknown, { params: { workspaceId: string; identifier: string } }>(
Expand Down Expand Up @@ -72,7 +72,7 @@ export const DELETE = request()
if (target == null) return false;
return can
.perform(Permission.TargetDelete)
.on({ type: "target", id: target.id });
.on({ type: "resource", id: target.id });
}),
)
.handle<unknown, { params: { workspaceId: string; identifier: string } }>(
Expand Down
2 changes: 1 addition & 1 deletion packages/api/src/router/deployment-variable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ export const deploymentVariableRouter = createTRPCRouter({
authorizationCheck: ({ canUser, input }) =>
canUser
.perform(Permission.DeploymentGet)
.on({ type: "target", id: input }),
.on({ type: "resource", id: input }),
})
.input(z.string().uuid())
.query(async ({ ctx, input }) => {
Expand Down
4 changes: 2 additions & 2 deletions packages/api/src/router/deployment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ export const deploymentRouter = createTRPCRouter({
r.map((row) => ({
...row.latest_jobs,
release: row.release,
target: row.target,
target: row.resource,
releaseJobTrigger: row.release_job_trigger,
})),
);
Expand Down Expand Up @@ -410,7 +410,7 @@ export const deploymentRouter = createTRPCRouter({
authorizationCheck: ({ canUser, input }) =>
canUser
.perform(Permission.DeploymentList)
.on({ type: "target", id: input }),
.on({ type: "resource", id: input }),
})
.query(async ({ ctx, input }) => {
const tg = await ctx.db
Expand Down
16 changes: 8 additions & 8 deletions packages/api/src/router/job.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ const processReleaseJobTriggerWithAdditionalDataRows = (
rows: Array<{
release_job_trigger: ReleaseJobTrigger;
job: Job;
target: Target;
resource: Target;
release: Release;
deployment: Deployment;
environment: Environment;
Expand Down Expand Up @@ -110,7 +110,7 @@ const processReleaseJobTriggerWithAdditionalDataRows = (
.value(),
},
jobAgent: v[0]!.job_agent,
target: v[0]!.target,
target: v[0]!.resource,
release: { ...v[0]!.release, deployment: v[0]!.deployment },
environment: v[0]!.environment,
releaseDependencies: v
Expand Down Expand Up @@ -173,7 +173,7 @@ const releaseJobTriggerRouter = createTRPCRouter({
...t.release_job_trigger,
job: t.job,
agent: t.job_agent,
target: t.target,
target: t.resource,
release: { ...t.release, deployment: t.deployment },
environment: t.environment,
})),
Expand Down Expand Up @@ -291,7 +291,7 @@ const releaseJobTriggerRouter = createTRPCRouter({
...t.release_job_trigger,
job: t.job,
jobAgent: t.job_agent,
target: t.target,
target: t.resource,
release: { ...t.release, deployment: t.deployment },
environment: t.environment,
})),
Expand Down Expand Up @@ -387,7 +387,7 @@ const releaseJobTriggerRouter = createTRPCRouter({
NULL::uuid AS tr_id,
NULL::uuid AS source_id,
NULL::uuid AS target_id,
NULL::target_relationship_type AS type
NULL::resource_relationship_type AS type
UNION ALL
-- Recursive case: find all relationships connected to the current set of IDs
SELECT
Expand All @@ -404,7 +404,7 @@ const releaseJobTriggerRouter = createTRPCRouter({
tr.target_id,
tr.type
FROM reachable_relationships rr
JOIN target_relationship tr ON tr.source_id = rr.id OR tr.target_id = rr.id
JOIN resource_relationship tr ON tr.source_id = rr.id OR tr.target_id = rr.id
WHERE
NOT CASE
WHEN tr.source_id = rr.id THEN tr.target_id
Expand Down Expand Up @@ -617,7 +617,7 @@ export const jobRouter = createTRPCRouter({
authorizationCheck: ({ canUser, input }) =>
canUser
.perform(Permission.SystemList)
.on({ type: "target", id: input }),
.on({ type: "resource", id: input }),
})
.input(z.string())
.query(({ ctx, input }) =>
Expand All @@ -630,7 +630,7 @@ export const jobRouter = createTRPCRouter({
...t.release_job_trigger,
job: t.job,
agent: t.job_agent,
target: t.target,
target: t.resource,
deployment: t.deployment,
release: { ...t.release },
environment: t.environment,
Expand Down
2 changes: 1 addition & 1 deletion packages/api/src/router/release-deploy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ export const releaseDeployRouter = createTRPCRouter({
.perform(Permission.ReleaseGet, Permission.TargetUpdate)
.on(
{ type: "release", id: input.releaseId },
{ type: "target", id: input.targetId },
{ type: "resource", id: input.targetId },
),
})
.input(
Expand Down
2 changes: 1 addition & 1 deletion packages/api/src/router/release.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ export const releaseRouter = createTRPCRouter({
.map((j) => ({
...j.release_job_trigger,
job: j.job,
target: j.target,
target: j.resource,
Copy link
Contributor

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 the target -> resource refactoring

While the value has been updated to use j.resource, the property name is still using the old "target" terminology. To maintain consistency with the broader refactoring effort, the property name should also be updated.

Apply this change:

-              target: j.resource,
+              resource: j.resource,
📝 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.

Suggested change
target: j.resource,
resource: j.resource,

})),
}));
};
Expand Down
6 changes: 3 additions & 3 deletions packages/api/src/router/target-metadata-group.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ export const targetMetadataGroupRouter = createTRPCRouter({
authorizationCheck: ({ canUser, input }) =>
canUser
.perform(Permission.TargetMetadataGroupGet)
.on({ type: "targetMetadataGroup", id: input }),
.on({ type: "resourceMetadataGroup", id: input }),
})
.input(z.string().uuid())
.query(async ({ ctx, input }) => {
Expand Down Expand Up @@ -209,7 +209,7 @@ export const targetMetadataGroupRouter = createTRPCRouter({
authorizationCheck: ({ canUser, input }) =>
canUser
.perform(Permission.TargetMetadataGroupUpdate)
.on({ type: "targetMetadataGroup", id: input.id }),
.on({ type: "resourceMetadataGroup", id: input.id }),
})
.input(
z.object({
Expand All @@ -231,7 +231,7 @@ export const targetMetadataGroupRouter = createTRPCRouter({
authorizationCheck: ({ canUser, input }) =>
canUser
.perform(Permission.TargetMetadataGroupDelete)
.on({ type: "targetMetadataGroup", id: input }),
.on({ type: "resourceMetadataGroup", id: input }),
})
.input(z.string().uuid())
.mutation(({ ctx, input }) =>
Expand Down
18 changes: 9 additions & 9 deletions packages/api/src/router/target-provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export const targetProviderRouter = createTRPCRouter({
.where(
inArray(
target.providerId,
providers.map((p) => p.target_provider.id),
providers.map((p) => p.resource_provider.id),
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Critical: Schema and property access mismatch

The code attempts to access properties using resource_provider and resource_provider_google, but the schema imports still use targetProvider and targetProviderGoogle. This mismatch will likely cause runtime errors.

Apply this fix to align with the imported schema names:

- providers.map((p) => p.resource_provider.id)
+ providers.map((p) => p.target_provider.id)

- ...provider.resource_provider,
- googleConfig: provider.resource_provider_google,
+ ...provider.target_provider,
+ googleConfig: provider.target_provider_google,

- (pc) => pc.providerId === provider.resource_provider.id,
+ (pc) => pc.providerId === provider.target_provider.id,

- .filter((pk) => pk.providerId === provider.resource_provider.id)
+ .filter((pk) => pk.providerId === provider.target_provider.id)

Also applies to: 64-64, 71-72, 75-75, 78-78

),
)
.groupBy(target.providerId);
Expand All @@ -61,21 +61,21 @@ export const targetProviderRouter = createTRPCRouter({
.where(
inArray(
target.providerId,
providers.map((p) => p.target_provider.id),
providers.map((p) => p.resource_provider.id),
),
)
.groupBy(target.providerId, target.kind, target.version)
.orderBy(sql`count(*) DESC`);

return providers.map((provider) => ({
...provider.target_provider,
googleConfig: provider.target_provider_google,
...provider.resource_provider,
googleConfig: provider.resource_provider_google,
targetCount:
providerCounts.find(
(pc) => pc.providerId === provider.target_provider.id,
(pc) => pc.providerId === provider.resource_provider.id,
)?.count ?? 0,
kinds: providerKinds
.filter((pk) => pk.providerId === provider.target_provider.id)
.filter((pk) => pk.providerId === provider.resource_provider.id)
.map(({ kind, version, count }) => ({ kind, version, count })),
}));
}),
Expand All @@ -85,7 +85,7 @@ export const targetProviderRouter = createTRPCRouter({
authorizationCheck: ({ canUser, input }) =>
canUser
.perform(Permission.TargetList)
.on({ type: "targetProvider", id: input }),
.on({ type: "resourceProvider", id: input }),
})
.input(z.string().uuid())
.query(({ ctx, input }) =>
Expand All @@ -102,7 +102,7 @@ export const targetProviderRouter = createTRPCRouter({
authorizationCheck: ({ canUser, input }) =>
canUser
.perform(Permission.TargetProviderUpdate)
.on({ type: "targetProvider", id: input }),
.on({ type: "resourceProvider", id: input }),
})
.input(z.string().uuid())
.mutation(async ({ input }) =>
Expand Down Expand Up @@ -209,7 +209,7 @@ export const targetProviderRouter = createTRPCRouter({
authorizationCheck: ({ canUser, input }) =>
canUser
.perform(Permission.TargetDelete)
.on({ type: "targetProvider", id: input.providerId }),
.on({ type: "resourceProvider", id: input.providerId }),
})
.input(
z.object({
Expand Down
Loading
Loading