paulononato/backstage
1
0
Fork 0
Code Issues Pull Requests Actions 32 Packages Projects Releases Wiki Activity

address review comments

Browse Source 
Signed-off-by: Kashish Mittal <kmittal@redhat.com>
This commit is contained in:
Kashish Mittal
2025-04-28 11:27:59 -04:00
parent 66f50e6a42
commit d2b8e6e461
14 changed files with 21 additions and 70 deletions
Download Patch File Download Diff File Expand all files Collapse all files
plugins/scaffolder-backend/report-alpha.api.md
+1 -2
View File
@@ -11,7 +11,6 @@ import { PermissionCriteria } from '@backstage/plugin-permission-common';
import { PermissionRule } from '@backstage/plugin-permission-node';
import { ResourcePermission } from '@backstage/plugin-permission-common';
import { SerializedTask } from '@backstage/plugin-scaffolder-node';
import { TaskFilter } from '@backstage/plugin-scaffolder-node';
import { TemplateEntityStepV1beta3 } from '@backstage/plugin-scaffolder-common';
import { TemplateParametersV1beta3 } from '@backstage/plugin-scaffolder-common';
@@ -89,7 +88,7 @@ export const scaffolderTaskConditions: Conditions<{
hasCreatedBy: PermissionRule<
SerializedTask,
{
property: TaskFilter['property'];
key: string;
values: any;
},
'scaffolder-task',
plugins/scaffolder-backend/report.api.md
-4
View File
@@ -329,8 +329,6 @@ export class DatabaseTaskStore implements TaskStore {
// (undocumented)
getTask(taskId: string): Promise<SerializedTask>;
// (undocumented)
getTasks(taskIds: string[]): Promise<SerializedTask_2[]>;
// (undocumented)
getTaskState({ taskId }: { taskId: string }): Promise<
| {
state: JsonObject;
@@ -485,8 +483,6 @@ export interface TaskStore {
// (undocumented)
getTask(taskId: string): Promise<SerializedTask>;
// (undocumented)
getTasks(taskIds: string[]): Promise<SerializedTask[]>;
// (undocumented)
getTaskState?({ taskId }: { taskId: string }): Promise<
| {
state: JsonObject;
plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.test.ts
+1 -1
View File
@@ -263,7 +263,7 @@ describe('DatabaseTaskStore', () => {
const permissionFilters: PermissionCriteria<TaskFilters> = {
not: {
property: 'createdBy',
key: 'created_by',
values: ['user:default/three', 'user:default/four'],
},
};
plugins/scaffolder-backend/src/scaffolder/tasks/DatabaseTaskStore.ts
+6 -26
View File
@@ -205,7 +205,7 @@ export class DatabaseTaskStore implements TaskStore {
}
private isTaskFilter(filter: any): filter is TaskFilter {
return filter.hasOwnProperty('property');
return filter.hasOwnProperty('key');
}
private parseFilter(
@@ -214,20 +214,16 @@ export class DatabaseTaskStore implements TaskStore {
db: Knex,
negate: boolean = false,
): Knex.QueryBuilder {
// handle not criteria
if (isNotCriteria(filter)) {
return this.parseFilter(filter.not, query, db, !negate);
}
if (this.isTaskFilter(filter)) {
const values: string[] = compact(filter.values) ?? [];
if (filter.property === 'createdBy') {
if (negate) {
query.whereNotIn('created_by', [...new Set(values)]);
} else {
query.whereIn('created_by', [...new Set(values)]);
}
if (negate) {
query.whereNotIn(filter.key, values);
} else {
query.whereIn(filter.key, values);
}
return query;
@@ -279,7 +275,7 @@ export class DatabaseTaskStore implements TaskStore {
createdByValues.length > 0
? {
allOf: [
{ property: 'createdBy', values: createdByValues },
{ key: 'created_by', values: createdByValues },
...(permissionFilters ? [permissionFilters] : []),
],
}
@@ -347,22 +343,6 @@ export class DatabaseTaskStore implements TaskStore {
}
}
async getTasks(taskIds: string[]): Promise<SerializedTask[]> {
const results = await this.db<RawDbTaskRow>('tasks')
.whereIn('id', taskIds)
.select();
return results.map(result => {
try {
return this.parseTaskRow(result);
} catch (error) {
throw new Error(
`Failed to parse spec of task '${result.id}', ${error}`,
);
}
});
}
private parseTaskRow(result: RawDbTaskRow): SerializedTask {
const spec = JSON.parse(result.spec);
const secrets = result.secrets ? JSON.parse(result.secrets) : undefined;
plugins/scaffolder-backend/src/scaffolder/tasks/StorageTaskBroker.ts
-7
View File
@@ -403,13 +403,6 @@ export class StorageTaskBroker implements TaskBroker {
return this.storage.getTask(taskId);
}
/**
* {@inheritdoc TaskBroker.getTasks}
*/
getTasks(taskIds: string[]): Promise<SerializedTask[]> {
return this.storage.getTasks(taskIds);
}
/**
* {@inheritdoc TaskBroker.event$}
*/
plugins/scaffolder-backend/src/scaffolder/tasks/types.ts
-2
View File
@@ -110,8 +110,6 @@ export interface TaskStore {
getTask(taskId: string): Promise<SerializedTask>;
getTasks(taskIds: string[]): Promise<SerializedTask[]>;
claimTask(): Promise<SerializedTask | undefined>;
completeTask(options: {
plugins/scaffolder-backend/src/service/permissions.ts
+2 -3
View File
@@ -27,7 +27,6 @@ import {
import { PermissionRuleParams } from '@backstage/plugin-permission-common';
import {
SerializedTask,
TaskFilter,
} from '@backstage/plugin-scaffolder-node';
/**
@@ -82,8 +81,8 @@ export type TaskPermissionRuleInput<
> = PermissionRule<
SerializedTask,
{
property: TaskFilter['property'];
values: any;
key: string;
values?: string[];
},
typeof RESOURCE_TYPE_SCAFFOLDER_TASK,
TParams
plugins/scaffolder-backend/src/service/router.ts
+1 -1
View File
@@ -136,6 +136,7 @@ import {
} from '@backstage/plugin-scaffolder-node';
/**
* RouterOptions
*/
export interface RouterOptions {
@@ -366,7 +367,6 @@ export async function createRouter(
resourceType: RESOURCE_TYPE_SCAFFOLDER_TASK,
permissions: scaffolderTaskPermissions,
rules: taskRules,
getResources: resourceRefs => taskBroker.getTasks(resourceRefs),
},
],
permissions: scaffolderPermissions,
plugins/scaffolder-backend/src/service/rules.test.ts
+2 -2
View File
@@ -586,7 +586,7 @@ describe('hasCreatedBy', () => {
hasCreatedBy.toQuery({
createdBy: ['user:default/user-1'],
}),
).toEqual({ property: 'createdBy', values: ['user:default/user-1'] });
).toEqual({ key: 'created_by', values: ['user:default/user-1'] });
});
});
it('returns the correct query filter with values (multiple users in createdBy)', () => {
@@ -595,7 +595,7 @@ describe('hasCreatedBy', () => {
createdBy: ['user:default/user-1', 'user:default/user-2'],
}),
).toEqual({
property: 'createdBy',
key: 'created_by',
values: ['user:default/user-1', 'user:default/user-2'],
});
});
plugins/scaffolder-backend/src/service/rules.ts
+3 -3
View File
@@ -27,7 +27,7 @@ import {
TemplateParametersV1beta3,
} from '@backstage/plugin-scaffolder-common';
import { SerializedTask, TaskFilter } from '@backstage/plugin-scaffolder-node';
import { SerializedTask } from '@backstage/plugin-scaffolder-node';
import { z } from 'zod';
import { JsonObject, JsonPrimitive } from '@backstage/types';
@@ -136,7 +136,7 @@ function buildHasProperty<Schema extends z.ZodType<JsonPrimitive>>({
export const createTaskPermissionRule = makeCreatePermissionRule<
SerializedTask,
{
property: TaskFilter['property'];
key: string;
values: any;
},
typeof RESOURCE_TYPE_SCAFFOLDER_TASK
@@ -161,7 +161,7 @@ export const hasCreatedBy = createTaskPermissionRule({
},
toQuery: ({ createdBy }) => {
return {
property: 'createdBy' as TaskFilter['property'],
key: 'created_by',
values: createdBy,
};
},
plugins/scaffolder-node/report.api.md
+2 -4
View File
@@ -361,8 +361,6 @@ export interface TaskBroker {
// (undocumented)
get(taskId: string): Promise<SerializedTask>;
// (undocumented)
getTasks(taskIds: string[]): Promise<SerializedTask[]>;
// (undocumented)
list?(options?: {
filters?: {
createdBy?: string | string[];
@@ -470,8 +468,8 @@ export type TaskEventType = 'completion' | 'log' | 'cancelled' | 'recovered';
// @public
export type TaskFilter = {
property: 'createdBy';
values: Array<string> | undefined;
key: string;
values?: Array<string> | undefined;
};
// @public
plugins/scaffolder-node/src/tasks/types.ts
+2 -4
View File
@@ -110,8 +110,8 @@ export type TaskBrokerDispatchOptions = {
* @public
*/
export type TaskFilter = {
property: 'createdBy';
values: Array<string> | undefined;
key: string;
values?: Array<string> | undefined;
};
/**
@@ -204,8 +204,6 @@ export interface TaskBroker {
get(taskId: string): Promise<SerializedTask>;
getTasks(taskIds: string[]): Promise<SerializedTask[]>;
list?(options?: {
filters?: {
createdBy?: string | string[];
plugins/scaffolder/src/components/OngoingTask/OngoingTask.test.tsx
-10
View File
@@ -161,16 +161,6 @@ describe('OngoingTask', () => {
await expect(rendered.findByText('Hide Logs')).resolves.toBeInTheDocument();
});
it('should render not found error page when user does not have permission to read the task', async () => {
const permissionApi = mockApis.permission({
authorize: AuthorizeResult.DENY,
});
await expect(render(permissionApi)).rejects.toThrow(
'Reached NotFound Page',
);
});
it('should have cancel button be disabled when user has read permission but lacks cancel permission', async () => {
const permissionApi = mockApis.permission({
authorize: request => {
plugins/scaffolder/src/components/OngoingTask/OngoingTask.tsx
+1 -1
View File
@@ -46,7 +46,7 @@ import {
TaskSteps,
} from '@backstage/plugin-scaffolder-react/alpha';
import { useAsync } from '@react-hookz/web';
import { usePermission} from '@backstage/plugin-permission-react';
import { usePermission } from '@backstage/plugin-permission-react';
import {
taskCancelPermission,
taskCreatePermission,