diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 06d136b..b3f8018 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -30,3 +30,21 @@ Approved examples: - Do not implement readability-reducing compatibility alias patterns that accept multiple names for the same input. - For any given input/option, accept a single canonical name and update callers/tests/docs to match it instead of adding fallback aliases or normalization logic. - Do not introduce duplicate code; extract shared logic into centralized utilities/modules and update call sites instead of copy-pasting implementations. +- Add JSDoc headers to all exported functions and exported class methods; keep the docblocks concise and accurate. +- When changing, adding, or removing any MDX component or any behavior behind an MDX component, also update [src/app/docs/author-docs.mdx](src/app/docs/author-docs.mdx) so the documentation and smoke-test usage examples stay in sync. + +## Change Validation Rules + +- After any code change, run the relevant tests immediately. +- Do not auto-fix failing tests without user review. +- If tests fail after a change: + 1. Stop and present the failure details + 2. List options for resolution + 3. Assess whether the change is breaking to existing users/clients + 4. Wait for explicit user direction before proceeding with fixes +- Consider a change breaking if: + - Existing code/integrations will stop working + - Public APIs or interfaces are affected + - Client code will need updates to stay compatible + - User behavior or expectations will change unexpectedly +- This prevents silent compatibility issues and ensures informed decisions about changes. diff --git a/src/app/contexts/instances.tsx b/src/app/contexts/instances.tsx index ef0b18d..34d80ab 100644 --- a/src/app/contexts/instances.tsx +++ b/src/app/contexts/instances.tsx @@ -16,7 +16,13 @@ import { InstanceUdf, Protocol, } from "@/lib/types"; -import { getComponentName, getVariable, setClientVariable, setVariable } from "@/lib/variables"; +import { + getComponentName, + getVariable, + resolveTemplateStringValue, + setClientVariable, + setVariable, +} from "@/lib/variables"; import { useInstances } from "@/lib/client-variables"; import { syncDockerInstances } from "@/app/lib/docker-instance-sync"; import type { DockerSyncItem } from "@/app/lib/docker-instance-sync"; @@ -34,6 +40,18 @@ const resolveDockerEnvValues = async ( ): Promise => { return await Promise.all( env.map(async (entry) => { + const templateResolvedValue = + typeof entry.value === "string" + ? await resolveTemplateStringValue(entry.value) + : entry.value; + + if (typeof templateResolvedValue === "string" && templateResolvedValue !== entry.value) { + return { + ...entry, + value: templateResolvedValue, + }; + } + if (!entry.isVariable || (entry.value ?? "") !== "") { return entry; } diff --git a/src/app/docs/author-docs.mdx b/src/app/docs/author-docs.mdx index 54ff652..3ec30ee 100644 --- a/src/app/docs/author-docs.mdx +++ b/src/app/docs/author-docs.mdx @@ -299,14 +299,16 @@ The framework uses the **DockerContainer** wrapper component to help correctly d The **Docker** component is responsible for managing and displaying information about the container. -| Variable | Description | Required | -|-----------------|-------------------------------------------------------|----------| -| **name** | A unique name for the Docker container. | Yes | -| **description** | The description of the Docker container. | No | -| **image** | The image of the Docker container. | Yes | -| **env** | The environment variables for the Docker container. | No | -| **port** | The port mapping for the Docker container. | No | -| **attrs** | Additional attributes for the Docker container. | No | +| Variable | Description | Required | +|-----------------|-----------------------------------------------------------------------------|----------| +| **name** | A unique name for the Docker container. | Yes | +| **description** | The description of the Docker container. | No | +| **image** | The image of the Docker container. | Yes | +| **env** | The environment variables for the Docker container. | No | +| **port** | The port mapping for the Docker container. | No | +| **attrs** | Additional attributes for the Docker container. | No | + +Each environment entry supports the optional `resolveTemplates` flag. When set to `true`, template placeholders like `${PETNAME}` are resolved before the container is created. This behavior is intentionally opt-in and defaults to off. ### Examples @@ -340,7 +342,8 @@ The **Docker** component is responsible for managing and displaying information image="private-registry.nginx.com/nginx-plus/agent:debian" env={[ { name: "NGINX_AGENT_SERVER_HOST", value: "agent.connect.nginx.com" }, - { name: "NGINX_AGENT_SERVER_TOKEN", isVariable: true, isSecret: true } + { name: "NGINX_AGENT_SERVER_TOKEN", isVariable: true, isSecret: true }, + { name: "NGINX_AGENT_LABELS", value: "config-sync-group=${PETNAME}", resolveTemplates: true } ]} port={{host: 55055, container: 80}} /> @@ -350,6 +353,8 @@ The **Docker** component is responsible for managing and displaying information /> +The `${VAR}` syntax is only expanded when you set `resolveTemplates: true` on the environment entry. For example, `config-sync-group=${PETNAME}` resolves to the current stored value for `PETNAME` before the container is created only when that flag is enabled. +
--- diff --git a/src/app/lib/docker-lib.test.ts b/src/app/lib/docker-lib.test.ts index b767922..bd5e5aa 100644 --- a/src/app/lib/docker-lib.test.ts +++ b/src/app/lib/docker-lib.test.ts @@ -13,9 +13,13 @@ import { DockerPortMapping } from "@/lib/types"; import { Protocol } from "@/lib/types"; jest.mock("@/app/lib/utils"); -jest.mock("@/lib/variables", () => ({ - getEnvVariable: jest.fn(), -})); +jest.mock("@/lib/variables", () => { + const actual = jest.requireActual("@/lib/variables"); + return { + ...actual, + getEnvVariable: jest.fn(), + }; +}); const mockContainer = { inspect: jest.fn().mockResolvedValue({ State: { Status: "running" } }), @@ -341,6 +345,42 @@ describe("Docker Library", () => { expect(mockContainer.start).toHaveBeenCalled(); }); + it("should not resolve string templates unless explicitly enabled", async () => { + process.env.PETNAME = "demo-123"; + + await createContainer({ + attrs: [], + env: [{ name: "NGINX_AGENT_LABELS", value: "config-sync-group=${PETNAME}" }], + image: "test-image", + name: "test-name", + ports: [{ containerPort: 80, hostPort: 8080 }], + }); + + expect(mockDockerode.createContainer).toHaveBeenCalledWith( + expect.objectContaining({ + Env: ["NGINX_AGENT_LABELS=config-sync-group=${PETNAME}"], + }) + ); + }); + + it("should resolve string template env values before creating a container when enabled", async () => { + process.env.PETNAME = "demo-123"; + + await createContainer({ + attrs: [], + env: [{ name: "NGINX_AGENT_LABELS", value: "config-sync-group=${PETNAME}", resolveTemplates: true }], + image: "test-image", + name: "test-name", + ports: [{ containerPort: 80, hostPort: 8080 }], + }); + + expect(mockDockerode.createContainer).toHaveBeenCalledWith( + expect.objectContaining({ + Env: ["NGINX_AGENT_LABELS=config-sync-group=demo-123"], + }) + ); + }); + it("should resolve secret env vars server-side before creating a container", async () => { (getEnvVariable as jest.Mock).mockImplementation(async (key: string) => { if (key === "DEPLOYMENT_IDENTIFIER") { diff --git a/src/app/lib/docker-lib.ts b/src/app/lib/docker-lib.ts index 69188de..fc64ca3 100644 --- a/src/app/lib/docker-lib.ts +++ b/src/app/lib/docker-lib.ts @@ -3,7 +3,7 @@ import { exec as execCallback } from "child_process"; import Docker from "dockerode"; import { promisify } from "util"; import { getInstanceDeploymentName } from "./utils"; -import { getEnvVariable } from "@/lib/variables"; +import { getEnvVariable, resolveTemplateStringValue } from "@/lib/variables"; import { DockerAttribute, InstanceDockerEnv, @@ -198,9 +198,28 @@ async function removeStoppedExistingContainer( } } +/** + * Resolves environment variables with explicit opt-in template expansion. + * Template values are only interpolated when the entry sets resolveTemplates. + * + * @param {InstanceDockerEnv[]} env - The environment entries to resolve. + * @returns {Promise} The resolved environment entries. + */ async function resolveContainerEnvVars(env: InstanceDockerEnv[]): Promise { return await Promise.all( env.map(async (entry) => { + const templateResolvedValue = + typeof entry.value === "string" && entry.resolveTemplates + ? await resolveTemplateStringValue(entry.value) + : entry.value; + + if (typeof templateResolvedValue === "string" && templateResolvedValue !== entry.value) { + return { + ...entry, + value: templateResolvedValue, + }; + } + if (!entry.isVariable) { return entry; } diff --git a/src/lib/types.ts b/src/lib/types.ts index 225d2a4..11df28e 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -143,12 +143,14 @@ interface InstanceBase { * @property {string} [value] - The value of the environment variable. * @property {boolean} [isVariable] - Whether it is a variable. * @property {boolean} [isSecret] - Whether it is a secret. + * @property {boolean} [resolveTemplates] - Whether template placeholders like ${PETNAME} should be resolved before the container is created. */ export interface InstanceDockerEnv { name: string; value?: string; isVariable?: boolean; isSecret?: boolean; + resolveTemplates?: boolean; } export interface DockerAttribute { diff --git a/src/lib/variables.ts b/src/lib/variables.ts index 6d8e4f1..4717e4e 100644 --- a/src/lib/variables.ts +++ b/src/lib/variables.ts @@ -95,6 +95,13 @@ function setLocalVariablesMap(values: Record): void { notifyLocalStorageChange(LOCAL_VARIABLES_STORAGE_KEY); } +/** + * Stores a client-side variable in browser localStorage and notifies subscribers. + * + * @param {string} key - The variable key to store. + * @param {string} value - The value to store for the variable. + * @returns {void} + */ export function setClientVariable(key: string, value: string): void { if (!isBrowserEnvironment()) { return; @@ -233,6 +240,9 @@ export async function getDeploymentIdentifier(): Promise { */ export async function getVariable(name: string): Promise { const candidateKeys = getCandidateKeys(name); + const normalizedName = name.trim().toLowerCase(); + const isDeploymentAliasLookup = + normalizedName === "petname" || normalizedName === "deployment_identifier"; if (isBrowserEnvironment()) { const variableMap = getLocalVariablesMap(); @@ -242,27 +252,72 @@ export async function getVariable(name: string): Promise { return variableMap[key] as unknown as T; } } + + if (normalizedName === "deployment_identifier") { + const topLevelDeploymentIdentifier = getLocalStorageString(DEPLOYMENT_IDENTIFIER_KEY); + if (topLevelDeploymentIdentifier) { + return topLevelDeploymentIdentifier as unknown as T; + } + } + } + + try { + const directValue = await getVariableServer(name); + if (directValue !== null && directValue !== undefined) { + return directValue; + } + } catch { + // Fall through to the deployment-identifier alias when no direct variable is found. + } + + if (isBrowserEnvironment() && normalizedName === "petname") { + const topLevelDeploymentIdentifier = getLocalStorageString(DEPLOYMENT_IDENTIFIER_KEY); + if (topLevelDeploymentIdentifier) { + return topLevelDeploymentIdentifier as unknown as T; + } } - if (candidateKeys.includes(DEPLOYMENT_IDENTIFIER_KEY)) { + if (isDeploymentAliasLookup) { const deploymentIdentifier = await getDeploymentIdentifier(); if (deploymentIdentifier) { return deploymentIdentifier as T; } } - try { - return await getVariableServer(name); - } catch { - return null; + return null; +} + +/** + * Resolves template placeholders such as ${VAR_NAME} using the current variable store. + * + * @param {string | null | undefined} value - The raw template string to resolve. + * @returns {Promise} The resolved string value, or the original input when no placeholders are present. + */ +export async function resolveTemplateStringValue(value?: string | null): Promise { + if (typeof value !== "string" || value.length === 0) { + return value ?? undefined; + } + + const matches = Array.from(new Set([...value.matchAll(/\$\{([^}]+)\}/g)].map((match) => match[1]))); + if (matches.length === 0) { + return value; } + + let resolvedValue = value; + for (const variableName of matches) { + const variableValue = await getVariable(variableName); + const replacement = variableValue === null || variableValue === undefined ? "" : String(variableValue); + resolvedValue = resolvedValue.split(`\${${variableName}}`).join(replacement); + } + + return resolvedValue; } /** - * Sets a variable in browser local storage map (client) and process env (server). + * Persists a variable value both in local browser state and the server environment. * - * @param {string} key - The key of the variable to set - * @param {string} value - The value of the variable to set + * @param {string} key - The variable key to persist. + * @param {string} value - The value to assign to the variable. * @returns {Promise} */ export async function setVariable(key: string, value: string) {