[DRAFT] Implement constructive-sdk into function, fixes, and cleanup#8
[DRAFT] Implement constructive-sdk into function, fixes, and cleanup#8Jovonni wants to merge 37 commits into
Conversation
functions testing harness
Dev/testing strategy using a matrix
| RUN echo '{}' > package.json | ||
|
|
||
| # Install common runtime deps | ||
| RUN pnpm install express@4.18.2 body-parser@1.20.2 graphql-request@6.1.0 pg@8.11.3 cross-fetch@4.0.0 |
There was a problem hiding this comment.
what about a package.json? why install manualy?
|
|
||
| NOTE: ALL INDEX.TS MUST HAVE A TYPED GRAPHQL QUERY, PERIOD, DONT EVER USE STRING BASED GQL QUERY.. EEVR... | ||
|
|
||
| okay so how do we know everything belowis impemented and tested then? THEN ANALYSE ALL OF THE GIT STATUS AND GIT DIFFS TO EDUCATE ME ON WHAT WAS CHANGED AND HOW IT ALIGNS TO WHAT WAS ASKED OF ME BELOW: |
There was a problem hiding this comment.
let's clean up some english, looks fun maybe that helps llm lol — I'd just have an agent pass over to clean up grammer/spelling
| try { | ||
| // Priority 2: Runtime's node_modules (Base Image context) | ||
| return require(require.resolve(name, { paths: [__dirname] })); | ||
| } catch (e2) { |
There was a problem hiding this comment.
let's avoid require? and go in an TS?
| // 5. Setup Route | ||
| // 5. Setup Route | ||
| // 6. Start Server | ||
| const port = Number(process.env.PORT ?? 8080); |
There was a problem hiding this comment.
let's iron out our env stuff, we may need to cleanup 12factor-env but I think it should support env vars now
|
|
||
| // Use standard PG env vars | ||
| const pgClient = new Client({ | ||
| user: process.env.PGUSER || 'postgres', |
There was a problem hiding this comment.
we should also use our env vars from either pg-env or another — @Anmol1696 let's figure out how we want to extract some of the env stuff
|
|
||
| beforeAll(async () => { | ||
| // Start kubectl proxy in background to handle auth | ||
| const { spawn } = require('child_process'); |
There was a problem hiding this comment.
let's use import and hoist these
|
|
||
| // Connect to local proxy | ||
| k8s = new KubernetesClient({ | ||
| restEndpoint: 'http://127.0.0.1:8001' |
There was a problem hiding this comment.
endpoint like this should def be an env var
| { name: "PGHOST", value: "postgres" }, | ||
| { name: "PGUSER", value: "postgres" }, | ||
| { name: "PGDATABASE", value: "postgres" }, | ||
| { name: "PGPASSWORD", value: process.env.PGPASSWORD } |
There was a problem hiding this comment.
how are we aiming to handle secrets?
| if (podName) { | ||
| try { | ||
| // Use raw fetch via proxy because kubernetesjs might fail to parse text logs | ||
| const res = await fetch(`http://127.0.0.1:8001/api/v1/namespaces/${NAMESPACE}/pods/${podName}/log?tailLines=50`); |
There was a problem hiding this comment.
does k8s js have this route, I would imagine it does? why not import that and make it a bit more tight/clean? (cc @Anmol1696 )
| // We use process.env vars which should be injected by the runner/container | ||
| const client = new Client({ | ||
| host: process.env.PGHOST || 'localhost', | ||
| port: parseInt(process.env.PGPORT || '5432'), |
There was a problem hiding this comment.
we should be using pg-env, similar to before
| "access": "public", | ||
| "directory": "dist" | ||
| }, | ||
| "main": "dist/index.js", |
There was a problem hiding this comment.
dist/ should not be in main if publishConfig is set to dist
the publishConfig is correct, the main, types, and files are incorrect
| ['host', 'content-length', 'connection', 'content-type', 'accept', 'user-agent', 'accept-encoding'].forEach(k => delete safeHeaders[k]); | ||
|
|
||
| const sdk = createClient({ | ||
| endpoint: process.env.GRAPHQL_ENDPOINT || 'http://constructive-server:3000/graphql', |
There was a problem hiding this comment.
let's hoist all env vars to top of all functions
There was a problem hiding this comment.
Pull request overview
Integrates the typed @constructive-db/constructive-sdk across functions and standardizes builds/tests to reduce flakiness while adding local dev tooling for invoking and running functions.
Changes:
- Replaced
graphql-requestusage inside functions with@constructive-db/constructive-sdk(via sharedsdk.tgz). - Added local dev gateway + config loader + CLI invoker scripts.
- Updated K8s manifests, Dockerfiles, and k8s integration tests/test-runner image usage.
Reviewed changes
Copilot reviewed 107 out of 111 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/test-runner.ts | Updates test runner image/flags; adds kubectl clusterrolebinding creation. |
| scripts/invoke.ts | Adds a simple CLI to POST to a function endpoint. |
| scripts/gateway.ts | Adds a local Express gateway that auto-mounts functions. |
| scripts/config-loader.ts | Adds .env / config.json loader for function config. |
| package.json | Adds dev script and gateway/invoke dependencies. |
| k8s/overlays/local/kustomization.yaml | Adds local image overrides and Knative bypass annotation patches. |
| k8s/base/functions/stripe-function.yaml | Adds imagePullSecrets for GHCR pulls. |
| k8s/base/functions/simple-email.yaml | Adds imagePullSecrets for GHCR pulls. |
| k8s/base/functions/simple-bash.yaml | Adds imagePullSecrets for GHCR pulls. |
| k8s/base/functions/send-email-link.yaml | Adds imagePullSecrets for GHCR pulls. |
| k8s/base/functions/rust-hello-world.yaml | Adds imagePullSecrets for GHCR pulls. |
| k8s/base/functions/runtime-script.yaml | Adds imagePullSecrets for GHCR pulls. |
| k8s/base/functions/pytorch-gpu.yaml | Adds imagePullSecrets for GHCR pulls. |
| k8s/base/functions/opencode-headless.yaml | Adds imagePullSecrets for GHCR pulls. |
| k8s/base/functions/llm-internal-calvin.yaml | Adds imagePullSecrets for GHCR pulls. |
| k8s/base/functions/llm-external.yaml | Adds imagePullSecrets for GHCR pulls. |
| k8s/base/functions/github-repo-creator.yaml | Adds imagePullSecrets for GHCR pulls. |
| k8s/base/functions/crypto-login.yaml | Adds imagePullSecrets for GHCR pulls. |
| functions/twilio-sms/src/index.ts | Migrates to constructive SDK and header sanitization. |
| functions/twilio-sms/sdk.tgz | Points function to shared sdk.tgz. |
| functions/twilio-sms/package.json | Switches dependency to @constructive-db/constructive-sdk. |
| functions/twilio-sms/tests/index.test.ts | Bumps test runner image tag. |
| functions/twilio-sms/Dockerfile | Adds per-function Docker build using shared sdk.tgz. |
| functions/stripe-function/src/index.ts | Migrates to constructive SDK and header sanitization. |
| functions/stripe-function/sdk.tgz | Points function to shared sdk.tgz. |
| functions/stripe-function/package.json | Switches dependency to @constructive-db/constructive-sdk. |
| functions/stripe-function/tests/index.test.ts | Bumps test runner image tag and adds invocation evidence. |
| functions/stripe-function/Dockerfile | Adds per-function Docker build using shared sdk.tgz. |
| functions/simple-email/src/index.ts | Migrates to constructive SDK and header sanitization. |
| functions/simple-email/sdk.tgz | Points function to shared sdk.tgz. |
| functions/simple-email/package.json | Switches dependency to @constructive-db/constructive-sdk. |
| functions/simple-email/tests/index.test.ts | Bumps test runner image tag and adds invocation evidence. |
| functions/simple-email/Dockerfile | Updates Dockerfile copy paths + shared sdk.tgz. |
| functions/simple-bash/sdk.tgz | Points function to shared sdk.tgz. |
| functions/simple-bash/package.json | Renames package and adjusts publish/start metadata. |
| functions/simple-bash/tests/index.test.ts | Removes DB setup; bumps test runner image tag; uses /bin/sh. |
| functions/simple-bash/Dockerfile | Fixes COPY path for repo-root docker builds. |
| functions/send-email-link/src/index.ts | Migrates to constructive SDK; refactors DB/user lookups. |
| functions/send-email-link/sdk.tgz | Points function to shared sdk.tgz. |
| functions/send-email-link/package.json | Switches dependency to @constructive-db/constructive-sdk. |
| functions/send-email-link/tests/index.test.ts | Bumps test runner image tag and adds log capture. |
| functions/send-email-link/Dockerfile | Updates Dockerfile copy paths + shared sdk.tgz. |
| functions/rust-hello-world/sdk.tgz | Points function to shared sdk.tgz. |
| functions/rust-hello-world/package.json | Adds metadata/scripts for Rust function folder. |
| functions/rust-hello-world/tests/index.test.ts | Updates test comments re build responsibility. |
| functions/rust-hello-world/Dockerfile | Fixes COPY paths for repo-root docker builds. |
| functions/rust-hello-world/Cargo.lock | Adds Rust lockfile for reproducible builds. |
| functions/runtime-script/src/index.ts | Migrates to constructive SDK and header sanitization. |
| functions/runtime-script/sdk.tgz | Points function to shared sdk.tgz. |
| functions/runtime-script/package.json | Switches dependency to @constructive-db/constructive-sdk (+ graphql dep). |
| functions/runtime-script/tests/index.test.ts | Bumps test runner image tag and adds invocation evidence. |
| functions/runtime-script/Dockerfile | Adds per-function Docker build using shared sdk.tgz. |
| functions/pytorch-gpu/sdk.tgz | Points function to shared sdk.tgz. |
| functions/pytorch-gpu/package.json | Adds metadata/scripts for Python function folder. |
| functions/pytorch-gpu/tests/index.test.ts | Updates test comments re build responsibility. |
| functions/pytorch-gpu/Dockerfile | Fixes COPY paths for repo-root docker builds. |
| functions/pgpm-dump/src/index.ts | Migrates to constructive SDK and header sanitization. |
| functions/pgpm-dump/sdk.tgz | Points function to shared sdk.tgz. |
| functions/pgpm-dump/package.json | Switches dependency to @constructive-db/constructive-sdk. |
| functions/pgpm-dump/tests/index.test.ts | Bumps test runner image tag; adjusts env/log tailLines/verification. |
| functions/pgpm-dump/Dockerfile | Adds per-function Docker build using shared sdk.tgz. |
| functions/opencode-headless/src/index.ts | Migrates to constructive SDK and header sanitization. |
| functions/opencode-headless/sdk.tgz | Points function to shared sdk.tgz. |
| functions/opencode-headless/scripts/build-calvin.sh | Adds build fixes/patching for bun workspace deps. |
| functions/opencode-headless/package.json | Switches dependency to @constructive-db/constructive-sdk. |
| functions/opencode-headless/tests/index.test.ts | Bumps test runner image tag. |
| functions/opencode-headless/Dockerfile | Adds per-function Docker build using shared sdk.tgz. |
| functions/llm-internal-calvin/src/index.ts | Migrates to constructive SDK and header sanitization. |
| functions/llm-internal-calvin/sdk.tgz | Points function to shared sdk.tgz. |
| functions/llm-internal-calvin/package.json | Switches dependency to @constructive-db/constructive-sdk. |
| functions/llm-internal-calvin/tests/index.test.ts | Bumps test runner image tag and adjusts evidence logging. |
| functions/llm-internal-calvin/Dockerfile | Adds per-function Docker build using shared sdk.tgz. |
| functions/llm-external/src/index.ts | Migrates to constructive SDK; refactors provider handling. |
| functions/llm-external/sdk.tgz | Points function to shared sdk.tgz. |
| functions/llm-external/package.json | Switches dependency to @constructive-db/constructive-sdk (+ graphql dep). |
| functions/llm-external/tests/index.test.ts | Bumps test runner image tag and adds evidence logging. |
| functions/llm-external/Dockerfile | Adds per-function Docker build using shared sdk.tgz. |
| functions/hello-world/src/index.ts | Migrates to constructive SDK; returns user context. |
| functions/hello-world/sdk.tgz | Points function to shared sdk.tgz. |
| functions/hello-world/package.json | Fixes runner path; switches dependency to SDK (+ graphql dep). |
| functions/hello-world/tests/index.test.ts | Bumps test runner image tag; adds invocation/user-context verification. |
| functions/hello-world/Dockerfile | Adds multi-stage build + shared runtime usage. |
| functions/hello-world/.dockerignore | Adds function-level dockerignore. |
| functions/github-repo-creator/src/index.ts | Migrates to constructive SDK and header sanitization. |
| functions/github-repo-creator/sdk.tgz | Points function to shared sdk.tgz. |
| functions/github-repo-creator/package.json | Switches dependency to @constructive-db/constructive-sdk. |
| functions/github-repo-creator/tests/index.test.ts | Bumps test runner image tag and adds invocation evidence. |
| functions/github-repo-creator/Dockerfile | Adds per-function Docker build using shared sdk.tgz. |
| functions/crypto-login/src/index.ts | Migrates to constructive SDK and header sanitization. |
| functions/crypto-login/sdk.tgz | Points function to shared sdk.tgz. |
| functions/crypto-login/package.json | Switches dependency to @constructive-db/constructive-sdk. |
| functions/crypto-login/tests/index.test.ts | Bumps test runner image tag and adds invocation evidence. |
| functions/crypto-login/Dockerfile | Adds per-function Docker build using shared sdk.tgz. |
| functions/create-db/tsconfig.json | Adds TS config for new create-db function. |
| functions/create-db/src/index.ts | Adds a function that creates a Postgres database. |
| functions/create-db/sdk.tgz | Points function to shared sdk.tgz. |
| functions/create-db/package.json | Adds create-db function package metadata/scripts/deps. |
| functions/create-db/tests/run-k8s.sh | Switches test runner invocation to npx ts-node. |
| functions/create-db/tests/index.test.ts | Adds k8s integration test for create-db. |
| functions/create-db/Makefile | Adds per-function make targets (and image vars). |
| functions/create-db/Dockerfile | Adds per-function Docker build using shared sdk.tgz. |
| functions/_runtimes/sdk.tgz | Points runtime to shared sdk.tgz. |
| functions/_runtimes/node/runner.js | Enhances runtime dependency resolution + user/async flow context. |
| functions/_runtimes/node/Dockerfile.test | Adjusts test image cleanup steps. |
| functions/_runtimes/node/Dockerfile.runtime | Adds shared node runtime image for functions. |
| Makefile | Adds runtime build step, kind-load-all, bumps test runner tag, expands targets. |
| .gitignore | Keeps sdk.tgz tracked while ignoring other tgz files. |
| .dockerignore | Broadens node_modules ignore pattern. |
| .agent/rules/new-func-requirements.md | Adds agent guidance doc for function requirements. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - patch: |- | ||
| - op: add | ||
| path: /spec/template/metadata/annotations | ||
| value: | ||
| serving.knative.dev/bypass-tag-resolution: "true" |
There was a problem hiding this comment.
This JSON6902 patch adds /spec/template/metadata/annotations as a whole, which will replace the existing annotations map in the base Knative Service (timeouts/autoscaling/etc.). Patch the specific annotation key instead (e.g., add/replace /spec/template/metadata/annotations/serving.knative.dev~1bypass-tag-resolution) to avoid clobbering existing annotations.
| const value = process.env[name]; | ||
| if (!value) { | ||
| throw new Error(`Missing required environment variable ${name}`); | ||
| throw new Error(`Missing required environment variable ${name} `); | ||
| } |
There was a problem hiding this comment.
The thrown error message has a trailing space after the env var name, which can cause brittle string matching in logs/tests and looks like a typo. Remove the extra whitespace.
| const result = await sdk.api.findMany({ select: { id: true, name: true }, first: 1 }).execute(); | ||
| if (result.ok) console.error('[twilio-sms] GQL Response:', JSON.stringify(result.data, null, 2)); | ||
| if (!result.ok) { | ||
| console.error('[twilio-sms] GQL Request failed:', result.errors); |
There was a problem hiding this comment.
Successful GraphQL responses are logged with console.error, which will be treated as an error by log collectors/alerting. Use console.log/console.info for success paths and reserve console.error for failures.
| @@ -0,0 +1,22 @@ | |||
|
|
|||
| IMAGE_NAME ?= constructive/pgpm-dump-test:v1 | |||
There was a problem hiding this comment.
IMAGE_NAME is set to constructive/pgpm-dump-test:v1, which appears unrelated to the create-db function and looks like a copy/paste artifact. Either remove it if unused or update it to the correct create-db image name to avoid confusion.
| IMAGE_NAME ?= constructive/pgpm-dump-test:v1 |
| // Ensure default SA has permissions for test runner jobs | ||
| try { | ||
| spawnSync('kubectl', ['create', 'clusterrolebinding', 'default-admin', '--clusterrole=cluster-admin', '--serviceaccount=default:default'], { stdio: 'ignore' }); | ||
| } catch (e) { } |
There was a problem hiding this comment.
The test runner is granting cluster-admin to the default service account by creating a ClusterRoleBinding. This is a high-privilege, cluster-wide permission escalation that can persist beyond tests and affect other workloads. Prefer documenting RBAC as a prerequisite, or create a tightly-scoped Role/RoleBinding in the test namespace (and only when a dedicated flag is set), with idempotent checks (e.g., kubectl get clusterrolebinding ... / kubectl auth can-i).
| first: 5 | ||
| }).execute(); | ||
|
|
||
| const users = result.ok ? result.data : null; |
There was a problem hiding this comment.
Unused variable users.
| const users = result.ok ? result.data : null; |
| first: 5 | ||
| }).execute(); | ||
|
|
||
| const users = result.ok ? result.data : null; |
There was a problem hiding this comment.
Unused variable users.
| const users = result.ok ? result.data : null; |
| import axios from 'axios'; | ||
| import gql from 'graphql-tag'; | ||
| import { createClient } from '@constructive-db/constructive-sdk'; | ||
| import axios from 'axios'; // kept although fetch is used below? fetch is imported from cross-fetch. |
There was a problem hiding this comment.
Unused import axios.
| const safeHeaders = { ...headers }; | ||
| ['host', 'content-length', 'connection', 'content-type', 'accept', 'user-agent', 'accept-encoding'].forEach(k => delete safeHeaders[k]); | ||
|
|
||
| const sdk = createClient({ |
There was a problem hiding this comment.
Unused variable sdk.
|
|
||
| # The user's code will be mounted/copied here | ||
| WORKDIR /usr/src/app | ||
|
|
There was a problem hiding this comment.
The node runtime image runs processes as root because there is no USER directive to drop privileges, so any compromise of user code executed by runner.js results in full root access inside the container and increases the risk of container breakout on a shared cluster. To reduce impact, run the runtime under a dedicated unprivileged user (for example, the built-in node user) and ensure files and ports are configured so root is not required.
| # The user's code will be mounted/copied here | |
| WORKDIR /usr/src/app | |
| # Ensure the user code directory exists and is writable by the node user | |
| RUN mkdir -p /usr/src/app && chown -R node:node /usr/src/app | |
| # The user's code will be mounted/copied here | |
| WORKDIR /usr/src/app | |
| # Run the runtime as an unprivileged user | |
| USER node |
|
|
||
| // Invoke to trigger GQL log | ||
| console.log('[Test] Invoking github-repo-creator via proxy...'); | ||
| const proxyUrl = `http://127.0.0.1:8007/api/v1/namespaces/${NAMESPACE}/pods/${podName}:8080/proxy/`; |
There was a problem hiding this comment.
@Anmol1696 — all these k8s things to implement, is this really something we're going to allow?
@Jovonni I think we should ideally be writing these as code, not pinging our infra to spin up new things? I'm a bit confused on this one to be honest, maybe we can walk through it
| }); | ||
|
|
||
| // Execute GQL Query as proof of connectivity (without try-catch) | ||
| const result = await sdk.api.findMany({ select: { id: true, name: true }, first: 1 }).execute(); |
There was a problem hiding this comment.
how is needed this pgpm-dump? first api? Do you mean to get apis for a db?
| endpoint: metaGraphqlUrl, | ||
| headers: { | ||
| ...(process.env.GRAPHQL_AUTH_TOKEN ? { Authorization: `Bearer ${process.env.GRAPHQL_AUTH_TOKEN}` } : {}), | ||
| ...(process.env.META_GRAPHQL_HOST_HEADER ? { host: process.env.META_GRAPHQL_HOST_HEADER } : {}) |
There was a problem hiding this comment.
again, let's hoist these into a env.ts
| "private": true, | ||
| "private": false, | ||
| "main": "dist/index.js", | ||
| "types": "dist/index.d.ts", |
There was a problem hiding this comment.
dont' use dist/ in main, typesor files — jsut publishConfig.directory = dist
| client: GraphQLClient; | ||
| meta: GraphQLClient; | ||
| client: any; // Type as 'any' or SDK client type if strictly typed, but for now 'any' allows passing sdk | ||
| meta: any; |
| @@ -0,0 +1,55 @@ | |||
|
|
|||
| import dotenv from 'dotenv'; | |||
There was a problem hiding this comment.
let's avoid dotenv, creates a "works on my machine" scenarios
| import fs from 'fs'; | ||
| import path from 'path'; | ||
| import { GraphQLClient } from 'graphql-request'; | ||
|
|
There was a problem hiding this comment.
is this file for testing?
| import fetch from 'cross-fetch'; | ||
|
|
||
| const GATEWAY_URL = process.env.GATEWAY_URL || 'http://localhost:3000'; | ||
|
|
There was a problem hiding this comment.
is this file for testing?
pyramation
left a comment
pyramation
left a comment
There was a problem hiding this comment.
- A lot of hard-coded URLs that should be using k8s js
- but maybe we should be questioning the usage of calling k8s proxy in a function? (cc @Anmol1696 )
- lots of code that has mixed variables, paths, env vars scattered throughout code needs to be hoisted and systematized in a
env.tsfile - lots of JS style, we should be more TS and can tighten things up a lot
- small thing, but let's go with 2 spaces indent for formatting similar to our other repos and standards
- ideally everything uses a package.json, I saw a few manually npm installs in runners? maybe others?
- passwords all over, let's standardize this
- seeing lots of duplicated code, that is querying APIs, but not sure that it's doing what it should, e.g using a database id (e.g. pgpm-dump)
- found some
anytypes inGraphQLClient— there should be someway to fix that we can brainstorm - remove dotenv usage in favor of 12factor-env setup — dotenv creates a "works on my machine scenario" or let's at least strategize this so we can have cohesion
|
Awesome feedback! Thank you bro, will address all of these 🚀 |
Anmol1696
force-pushed
the
dev/functions-integrate-sdk
branch
from
cdfdf71 to
e714a8a
Compare
|
Heads-up: repository history was rewritten on 2026-05-12 to scrub leaked secrets (Postgres/pgAdmin default passwords, an AWS access key ID, generated This PR shows "DIRTY" / merge-conflict status because git fetch --all --prune
git checkout <this-branch>
git reset --hard origin/<this-branch> # your local branch tip moved too — pick up the rewritten version
git rebase origin/main # rebase onto rewritten main
# resolve conflicts (usually trivial — mostly secret-placeholder + the 4 deleted interweb-*.yaml files)
git push --force-with-leaseOr merge instead of rebase if the branch is shared with others: git merge origin/main
git pushNotes:
|
addresses:
summarized changes:
make test-k8s-all)successful tests
Details