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

Feat: Add approval and approval approver to openapi spec and get-job-status action #176

Merged
merged 9 commits into from
Oct 29, 2024
Merged

Feat: Add approval and approval approver to openapi spec and get-job-status action #176

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a85a25e
Add cauedBy to openapi spec
zacharyblasczyk Oct 27, 2024
629d1a8
Add approval and approval approver to openapi spec and get-job-inputs
zacharyblasczyk Oct 27, 2024
cb166f1
fix
zacharyblasczyk Oct 27, 2024
1c75f06
fix
zacharyblasczyk Oct 27, 2024
b5c9751
build --force
zacharyblasczyk Oct 27, 2024
3cf34e6
add approval section
zacharyblasczyk Oct 27, 2024
a49d333
Merge remote-tracking branch 'origin' into zacharyb/eng-256
zacharyblasczyk Oct 29, 2024
ee92450
reverting some changes
zacharyblasczyk Oct 29, 2024
23cde10
fix
zacharyblasczyk Oct 29, 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
117 changes: 114 additions & 3 deletions github/get-job-inputs/index.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

24 changes: 22 additions & 2 deletions integrations/github-get-job-inputs/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,16 @@
.then((response) => {
core.info(JSON.stringify(response, null, 2));

const { variables, target, release, environment, runbook, deployment } =
response;
const {
variables,
target,
release,
environment,
runbook,
deployment,
causedBy,
approval,
} = response;

setOutputAndLog("base_url", baseUrl);

Expand All @@ -75,11 +83,23 @@
setOutputsRecursively("release_config", release?.config);
setOutputsRecursively("release_metadata", release?.metadata);

setOutputAndLog("caused_by_id", causedBy.id);
setOutputAndLog("caused_by_name", causedBy.name);
setOutputAndLog("caused_by_email", causedBy.email);

setOutputAndLog("approval_status", approval.status);
setOutputAndLog("approval_approver_id", approval.approver?.id ?? "");
setOutputAndLog("approval_approver_name", approval.approver?.name ?? "");
setOutputAndLog(
"approval_approver_email",
approval.approver?.email ?? "",
);

setOutputAndLog("deployment_id", deployment?.id);
setOutputAndLog("deployment_name", deployment?.name);
setOutputAndLog("deployment_slug", deployment?.slug);

for (const [key, value] of Object.entries(variables ?? {})) {

Check failure on line 102 in integrations/github-get-job-inputs/src/index.ts

View workflow job for this annotation

GitHub Actions / Lint 

Unnecessary conditional, expected left-hand side of `??` operator to be possibly null or undefined
const sanitizedKey = key.replace(/[.\-/\s\t]+/g, "_");
setOutputAndLog(`variable_${sanitizedKey}`, value);
}
Expand Down
51 changes: 44 additions & 7 deletions openapi.v1.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,6 @@ paths:
invalid_integration,
external_run_not_found,
]

release:
type: object
properties:
Expand All @@ -323,7 +322,6 @@ paths:
config:
type: object
required: [id, version, metadata, config]

deployment:
type: object
properties:
Expand All @@ -338,7 +336,6 @@ paths:
jobAgentId:
type: string
required: [id, version, slug, systemId, jobAgentId]

runbook:
type: object
properties:
Expand All @@ -351,7 +348,6 @@ paths:
jobAgentId:
type: string
required: [id, name, systemId, jobAgentId]

target:
type: object
properties:
Expand Down Expand Up @@ -380,7 +376,6 @@ paths:
- workspaceId
- config
- metadata

environment:
type: object
properties:
Expand All @@ -391,15 +386,57 @@ paths:
systemId:
type: string
required: [id, name, systemId]

variables:
type: object
causedBy:
type: object
nullable: true
properties:
id:
type: string
name:
type: string
email:
type: string
required:
- id
- name
- email
approval:
type: object
nullable: true
properties:
id:
type: string
status:
type: string
enum: [pending, approved, rejected]
approver:
type: object
nullable: true
description: Null when status is pending, contains approver details when approved or rejected
properties:
id:
type: string
name:
type: string
email:
type: string
required:
- id
- name
- email
required:
- id
- status
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding timestamps to track approval/rejection events.

While the approval schema is well-structured, consider adding:

  • approvedAt or rejectedAt timestamp to track when the approval status changed
  • requestedAt timestamp to track when the approval was initially requested

This would help with audit trails and time-based analytics.

Here's the suggested addition to the schema:

                    properties:
                      id:
                        type: string
                      status:
                        type: string
                        enum: [pending, approved, rejected]
+                     requestedAt:
+                       type: string
+                       format: date-time
+                     statusUpdatedAt:
+                       type: string
+                       format: date-time
                      approver:
📝 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
approval:
type: object
nullable: true
properties:
id:
type: string
status:
type: string
enum: [pending, approved, rejected]
approver:
type: object
nullable: true
description: Null when status is pending, contains approver details when approved or rejected
properties:
id:
type: string
name:
type: string
email:
type: string
required:
- id
- name
- email
required:
- id
- status
approval:
type: object
nullable: true
properties:
id:
type: string
status:
type: string
enum: [pending, approved, rejected]
requestedAt:
type: string
format: date-time
statusUpdatedAt:
type: string
format: date-time
approver:
type: object
nullable: true
description: Null when status is pending, contains approver details when approved or rejected
properties:
id:
type: string
name:
type: string
email:
type: string
required:
- id
- name
- email
required:
- id
- status

Copy link
Member Author

Choose a reason for hiding this comment

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

@jsbroks, I had the exact same idea about adding a statusUpdatedAt to the table so we could show in the UI "approved/rejected at Date()" I just wanted to do that in a followup PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

required:
- id
- status
- createdAt
- updatedAt
- variable
- variables
- causedBy
- approval
patch:
summary: Update a job
operationId: updateJob
Expand Down
32 changes: 29 additions & 3 deletions packages/api/src/router/job.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import type {
Deployment,
Environment,
EnvironmentPolicy,
EnvironmentPolicyApproval,
EnvironmentPolicyReleaseWindow,
Job,
JobAgent,
Expand All @@ -19,11 +20,13 @@ import { isPresent } from "ts-is-present";
import { z } from "zod";

import {
alias,
and,
asc,
desc,
eq,
inArray,
isNotNull,
isNull,
notInArray,
sql,
Expand All @@ -34,6 +37,7 @@ import {
deployment,
environment,
environmentPolicy,
environmentPolicyApproval,
environmentPolicyReleaseWindow,
job,
jobAgent,
Expand All @@ -60,16 +64,30 @@ import { JobStatus } from "@ctrlplane/validators/jobs";

import { createTRPCRouter, protectedProcedure } from "../trpc";

const releaseJobTriggerQuery = (tx: Tx) =>
tx
const releaseJobTriggerQuery = (tx: Tx) => {
const approver = alias(user, "approver");

return tx
.select()
.from(releaseJobTrigger)
.innerJoin(job, eq(releaseJobTrigger.jobId, job.id))
.innerJoin(target, eq(releaseJobTrigger.targetId, target.id))
.innerJoin(release, eq(releaseJobTrigger.releaseId, release.id))
.innerJoin(deployment, eq(release.deploymentId, deployment.id))
.innerJoin(environment, eq(releaseJobTrigger.environmentId, environment.id))
.innerJoin(jobAgent, eq(jobAgent.id, deployment.jobAgentId));
.innerJoin(jobAgent, eq(jobAgent.id, deployment.jobAgentId))
.leftJoin(
environmentPolicyApproval,
eq(environmentPolicyApproval.releaseId, release.id),
)
.leftJoin(
approver,
and(
isNotNull(environmentPolicyApproval.userId),
eq(environmentPolicyApproval.userId, approver.id),
),
);
};

const processReleaseJobTriggerWithAdditionalDataRows = (
rows: Array<{
Expand All @@ -87,6 +105,8 @@ const processReleaseJobTriggerWithAdditionalDataRows = (
release_dependency?: ReleaseDependency | null;
deployment_name?: { deploymentName: string; deploymentId: string } | null;
job_variable?: JobVariable | null;
environment_policy_approval: EnvironmentPolicyApproval | null;
approver?: User | null;
}>,
) =>
_.chain(rows)
Expand Down Expand Up @@ -127,6 +147,12 @@ const processReleaseJobTriggerWithAdditionalDataRows = (
.filter(isPresent),
)
: null,
approval: v[0]!.environment_policy_approval
? {
...v[0]!.environment_policy_approval,
approver: v[0]!.approver,
}
: null,
}))
.value();

Expand Down
2 changes: 1 addition & 1 deletion packages/node-sdk/openapitools.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"v1": {
"generatorName": "typescript-fetch",
"output": "#{cwd}/src",
"glob": "../../openapi.yaml"
"glob": "../../openapi.v1.yaml"
}
}
}
Expand Down
Loading
Loading