From e11410f7b56b71efbf94a3e0c122f21fc03511a6 Mon Sep 17 00:00:00 2001 From: Vigneshraj Sekar Babu Date: Thu, 4 Jun 2026 16:53:44 -0700 Subject: [PATCH 1/2] fix: apply namespace ttl labels during native builds --- .../lib/__tests__/namespaceMetadata.test.ts | 100 +++++++++++++++ src/server/lib/kubernetes.ts | 109 +++++++++++++--- .../lib/nativeBuild/__tests__/index.test.ts | 116 ++++++++++++++++++ src/server/lib/nativeBuild/index.ts | 16 ++- src/server/lib/nativeBuild/utils.ts | 49 -------- src/server/services/__tests__/deploy.test.ts | 20 ++- src/server/services/build.ts | 4 +- src/server/services/deploy.ts | 12 +- 8 files changed, 346 insertions(+), 80 deletions(-) create mode 100644 src/server/lib/nativeBuild/__tests__/index.test.ts diff --git a/src/server/lib/__tests__/namespaceMetadata.test.ts b/src/server/lib/__tests__/namespaceMetadata.test.ts index a7088878..2b4aa6bd 100644 --- a/src/server/lib/__tests__/namespaceMetadata.test.ts +++ b/src/server/lib/__tests__/namespaceMetadata.test.ts @@ -18,6 +18,8 @@ var mockReadNamespace: jest.Mock; var mockCreateNamespace: jest.Mock; var mockPatchNamespace: jest.Mock; var mockGetAllConfigs: jest.Mock; +var mockGetLabels: jest.Mock; +var mockShellPromise: jest.Mock; jest.mock('@kubernetes/client-node', () => { const actual = jest.requireActual('@kubernetes/client-node'); @@ -49,12 +51,14 @@ jest.mock('@kubernetes/client-node', () => { jest.mock('server/services/globalConfig', () => { mockGetAllConfigs = jest.fn(); + mockGetLabels = jest.fn(); return { __esModule: true, default: { getInstance: jest.fn(() => ({ getAllConfigs: mockGetAllConfigs, + getLabels: mockGetLabels, })), }, }; @@ -69,6 +73,14 @@ jest.mock('server/lib/logger', () => ({ })), })); +jest.mock('server/lib/shell', () => { + mockShellPromise = jest.fn(); + + return { + shellPromise: (...args: unknown[]) => mockShellPromise(...args), + }; +}); + import { createOrUpdateNamespace } from '../kubernetes'; describe('createOrUpdateNamespace metadata', () => { @@ -78,12 +90,18 @@ describe('createOrUpdateNamespace metadata', () => { mockCreateNamespace.mockReset(); mockPatchNamespace.mockReset(); mockGetAllConfigs.mockReset(); + mockGetLabels.mockReset(); + mockShellPromise.mockReset(); jest.useFakeTimers().setSystemTime(new Date('2026-04-16T12:00:00.000Z')); mockGetAllConfigs.mockResolvedValue({ ttl_cleanup: { inactivityDays: 7, }, }); + mockGetLabels.mockResolvedValue({ + keep: ['keep-env'], + }); + mockShellPromise.mockResolvedValue('Active'); }); afterEach(() => { @@ -120,6 +138,88 @@ describe('createOrUpdateNamespace metadata', () => { ); }); + it('adds TTL labels immediately for build-backed namespaces', async () => { + mockReadNamespace.mockRejectedValueOnce({ response: { statusCode: 404 } }); + mockCreateNamespace.mockResolvedValue({}); + + await createOrUpdateNamespace({ + name: 'env-build123', + buildUUID: 'build123', + staticEnv: false, + pullRequest: { + fullName: 'example-org/example-repo', + pullRequestNumber: 42, + githubLogin: 'example-author', + labels: ['deploy-env'], + }, + }); + + expect(mockCreateNamespace).toHaveBeenCalledWith( + expect.objectContaining({ + metadata: expect.objectContaining({ + name: 'env-build123', + labels: expect.objectContaining({ + 'lfc/uuid': 'build123', + 'lfc/type': 'ephemeral', + 'lfc/ttl-enable': 'true', + 'lfc/ttl-createdAtUnix': '1776340800000', + 'lfc/ttl-createdAt': '2026-04-16', + 'lfc/ttl-expireAtUnix': '1776945600000', + 'lfc/ttl-expireAt': '2026-04-23', + 'lfc/org': 'example-org', + 'lfc/repo': 'example-repo', + 'lfc/pull-request': '42', + 'lfc/author': 'example-author', + }), + }), + }) + ); + }); + + it('disables TTL for build-backed namespaces when the pull request has the keep label', async () => { + mockReadNamespace.mockRejectedValueOnce({ response: { statusCode: 404 } }); + mockCreateNamespace.mockResolvedValue({}); + + await createOrUpdateNamespace({ + name: 'env-keep123', + buildUUID: 'keep123', + staticEnv: false, + pullRequest: { + fullName: 'example-org/example-repo', + pullRequestNumber: 99, + githubLogin: 'example-author', + labels: ['keep-env'], + }, + }); + + expect(mockCreateNamespace).toHaveBeenCalledWith( + expect.objectContaining({ + metadata: expect.objectContaining({ + labels: expect.objectContaining({ + 'lfc/uuid': 'keep123', + 'lfc/type': 'ephemeral', + 'lfc/ttl-enable': 'false', + }), + }), + }) + ); + expect(mockCreateNamespace.mock.calls[0][0].metadata.labels).not.toHaveProperty('lfc/ttl-expireAtUnix'); + }); + + it('waits for the namespace to become active when requested', async () => { + mockReadNamespace.mockRejectedValueOnce({ response: { statusCode: 404 } }); + mockCreateNamespace.mockResolvedValue({}); + + await createOrUpdateNamespace({ + name: 'env-wait123', + buildUUID: 'wait123', + staticEnv: false, + waitForReady: true, + }); + + expect(mockShellPromise).toHaveBeenCalledWith("kubectl get namespace env-wait123 -o jsonpath='{.status.phase}'"); + }); + it('patches PR and repo labels onto an existing namespace', async () => { mockReadNamespace .mockResolvedValueOnce({ body: { metadata: { name: 'env-abc123', labels: {} } } }) diff --git a/src/server/lib/kubernetes.ts b/src/server/lib/kubernetes.ts index 805f0dc1..07f4cfeb 100644 --- a/src/server/lib/kubernetes.ts +++ b/src/server/lib/kubernetes.ts @@ -20,7 +20,7 @@ import _ from 'lodash'; import { Build, Deploy, Deployable, Service } from 'server/models'; import { CLIDeployTypes, KubernetesDeployTypes, MEDIUM_TYPE, DEFAULT_TTL_INACTIVITY_DAYS } from 'shared/constants'; import { shellPromise } from './shell'; -import { flattenObject, waitUntil } from 'server/lib/utils'; +import { flattenObject, getKeepLabel, waitUntil } from 'server/lib/utils'; import { ServiceDiskConfig } from 'server/models/yaml'; import * as k8s from '@kubernetes/client-node'; import { HttpError, V1Status, CoreV1Api, KubeConfig } from '@kubernetes/client-node'; @@ -82,6 +82,44 @@ type NamespaceMetadata = { labels: Record; }; +type NamespacePullRequestMetadata = { + fullName?: string | null; + pullRequestNumber?: number | null; + githubLogin?: string | null; + labels?: string[] | string | null; +}; + +export type CreateOrUpdateNamespaceOptions = { + name: string; + buildUUID: string; + staticEnv: boolean; + ttl?: boolean; + repo?: string | null; + pullRequestNumber?: number | null; + author?: string | null; + pullRequest?: NamespacePullRequestMetadata | null; + waitForReady?: boolean; +}; + +async function waitForNamespaceReady(namespace: string, timeout: number = 30000): Promise { + const startTime = Date.now(); + + while (Date.now() - startTime < timeout) { + try { + const result = await shellPromise(`kubectl get namespace ${namespace} -o jsonpath='{.status.phase}'`); + if (result.trim() === 'Active') { + return; + } + } catch (error) { + // Namespace not ready yet, will retry + } + + await new Promise((resolve) => setTimeout(resolve, 1000)); + } + + throw new Error(`Namespace ${namespace} did not become ready within ${timeout}ms`); +} + function buildNamespacePrMetadata({ repo, pullRequestNumber, @@ -114,6 +152,35 @@ function buildNamespacePrMetadata({ return { labels }; } +function parseNamespacePullRequestLabels(labels?: string[] | string | null): string[] { + if (!labels) { + return []; + } + + if (Array.isArray(labels)) { + return labels; + } + + try { + const parsed = JSON.parse(labels); + return Array.isArray(parsed) ? parsed : []; + } catch { + return []; + } +} + +async function shouldEnableNamespaceTTL( + staticEnv: boolean, + pullRequest?: NamespacePullRequestMetadata | null +): Promise { + if (staticEnv) { + return false; + } + + const keepLabel = await getKeepLabel(); + return !parseNamespacePullRequestLabels(pullRequest?.labels).includes(keepLabel); +} + /** * Generates TTL-related labels for namespace creation */ @@ -262,34 +329,32 @@ export async function createOrUpdateNamespace({ name, buildUUID, staticEnv, - ttl = true, + ttl, repo, pullRequestNumber, author, -}: { - name: string; - buildUUID: string; - staticEnv: boolean; - ttl?: boolean; - repo?: string | null; - pullRequestNumber?: number | null; - author?: string | null; -}) { + pullRequest, + waitForReady = false, +}: CreateOrUpdateNamespaceOptions) { const kc = new k8s.KubeConfig(); kc.loadFromDefault(); const client = kc.makeApiClient(k8s.CoreV1Api); const uuid = name.replace('env-', ''); + const ttlEnabled = ttl ?? (pullRequest ? await shouldEnableNamespaceTTL(staticEnv, pullRequest) : !staticEnv); + const namespaceRepo = repo ?? pullRequest?.fullName; + const namespacePullRequestNumber = pullRequestNumber ?? pullRequest?.pullRequestNumber; + const namespaceAuthor = author ?? pullRequest?.githubLogin; // Generate TTL labels using helper function const { labels, logMessage } = await generateTTLLabels({ uuid, staticEnv, - ttl, + ttl: ttlEnabled, buildUUID, - repo, - pullRequestNumber, - author, + repo: namespaceRepo, + pullRequestNumber: namespacePullRequestNumber, + author: namespaceAuthor, }); getLogger({ namespace: name }).info(`Deploy: creating namespace ${logMessage}`); @@ -307,23 +372,29 @@ export async function createOrUpdateNamespace({ const { patch, logMessage: patchMessage } = await generateTTLPatch({ uuid, staticEnv, - ttl: staticEnv ? false : ttl, + ttl: staticEnv ? false : ttlEnabled, buildUUID, - repo, - pullRequestNumber, - author, + repo: namespaceRepo, + pullRequestNumber: namespacePullRequestNumber, + author: namespaceAuthor, }); await client.patchNamespace(name, patch, undefined, undefined, undefined, undefined, undefined, { headers: { 'Content-Type': 'application/json-patch+json' }, }); getLogger({ namespace: name }).info(`Deploy: updated namespace ${patchMessage}`); + if (waitForReady) { + await waitForNamespaceReady(name); + } return; } try { await client.createNamespace(namespace); getLogger({ namespace: name }).debug('Namespace created'); + if (waitForReady) { + await waitForNamespaceReady(name); + } } catch (err) { getLogger({ namespace: name, error: err }).error('Namespace: create failed'); throw err; diff --git a/src/server/lib/nativeBuild/__tests__/index.test.ts b/src/server/lib/nativeBuild/__tests__/index.test.ts new file mode 100644 index 00000000..320ab3cc --- /dev/null +++ b/src/server/lib/nativeBuild/__tests__/index.test.ts @@ -0,0 +1,116 @@ +/** + * Copyright 2026 Contributors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +const mockCreateOrUpdateNamespace = jest.fn(); +const mockEnsureServiceAccountForJob = jest.fn(); +const mockBuildWithEngine = jest.fn(); + +jest.mock('../../kubernetes', () => ({ + createOrUpdateNamespace: (...args: unknown[]) => mockCreateOrUpdateNamespace(...args), +})); + +jest.mock('../../kubernetes/common/serviceAccount', () => ({ + ensureServiceAccountForJob: (...args: unknown[]) => mockEnsureServiceAccountForJob(...args), +})); + +jest.mock('../engines', () => ({ + buildWithEngine: (...args: unknown[]) => mockBuildWithEngine(...args), +})); + +jest.mock('../../buildEngines', () => ({ + isNativeBuilderEngine: jest.fn(() => true), +})); + +jest.mock('../../logger', () => ({ + getLogger: jest.fn(() => ({ + info: jest.fn(), + error: jest.fn(), + debug: jest.fn(), + warn: jest.fn(), + })), + withLogContext: jest.fn((_ctx, fn) => fn()), + withSpan: jest.fn((_name, fn) => fn()), +})); + +import { buildWithNative } from '../index'; + +describe('buildWithNative', () => { + beforeEach(() => { + jest.clearAllMocks(); + mockEnsureServiceAccountForJob.mockResolvedValue('native-build-sa'); + mockBuildWithEngine.mockResolvedValue({ + success: true, + logs: 'Build completed', + jobName: 'sample-job', + }); + }); + + it('creates or updates the namespace with TTL metadata before building', async () => { + const pullRequest = { + fullName: 'example-org/example-repo', + pullRequestNumber: 123, + githubLogin: 'example-author', + labels: ['deploy-env'], + }; + const deploy = { + deployable: { + name: 'sample-service', + builder: { + engine: 'buildkit', + }, + }, + build: { + uuid: 'build123', + isStatic: false, + pullRequest, + }, + $fetchGraph: jest.fn().mockResolvedValue(undefined), + }; + const options = { + ecrRepo: 'sample-repo', + ecrDomain: '123456789012.dkr.ecr.us-west-2.amazonaws.com', + envVars: {}, + dockerfilePath: 'Dockerfile', + tag: 'sample-tag', + revision: 'abcdef1234567890', + repo: 'example-org/example-repo', + branch: 'feature-branch', + namespace: 'env-build123', + buildId: '1', + buildUuid: 'build123', + deployUuid: 'deploy123', + }; + + await buildWithNative(deploy as any, options); + + expect(deploy.$fetchGraph).toHaveBeenCalledWith('[build.[pullRequest]]'); + expect(mockCreateOrUpdateNamespace).toHaveBeenCalledWith({ + name: 'env-build123', + buildUUID: 'build123', + staticEnv: false, + pullRequest, + waitForReady: true, + }); + expect(mockEnsureServiceAccountForJob).toHaveBeenCalledWith('env-build123', 'build'); + expect(mockBuildWithEngine).toHaveBeenCalledWith( + deploy, + expect.objectContaining({ + serviceAccount: 'native-build-sa', + }), + 'buildkit' + ); + }); +}); diff --git a/src/server/lib/nativeBuild/index.ts b/src/server/lib/nativeBuild/index.ts index 28ead1c7..159c56f3 100644 --- a/src/server/lib/nativeBuild/index.ts +++ b/src/server/lib/nativeBuild/index.ts @@ -16,10 +16,10 @@ import { Deploy } from '../../models'; import { getLogger, withSpan, withLogContext } from '../logger'; -import { ensureNamespaceExists } from './utils'; import { buildWithEngine, NativeBuildOptions } from './engines'; import { ensureServiceAccountForJob } from '../kubernetes/common/serviceAccount'; import { isNativeBuilderEngine } from '../buildEngines'; +import { createOrUpdateNamespace } from '../kubernetes'; export type { NativeBuildOptions } from './engines'; @@ -38,7 +38,19 @@ export async function buildWithNative(deploy: Deploy, options: NativeBuildOption getLogger().info('Build: starting (native)'); try { - await ensureNamespaceExists(options.namespace); + await deploy.$fetchGraph('[build.[pullRequest]]'); + + if (!deploy.build) { + throw new Error('Build: namespace setup requires build metadata'); + } + + await createOrUpdateNamespace({ + name: options.namespace, + buildUUID: deploy.build.uuid, + staticEnv: deploy.build.isStatic, + pullRequest: deploy.build.pullRequest, + waitForReady: true, + }); const serviceAccountName = await ensureServiceAccountForJob(options.namespace, 'build'); diff --git a/src/server/lib/nativeBuild/utils.ts b/src/server/lib/nativeBuild/utils.ts index aeb453a9..57acdf72 100644 --- a/src/server/lib/nativeBuild/utils.ts +++ b/src/server/lib/nativeBuild/utils.ts @@ -15,60 +15,11 @@ */ import { V1Job } from '@kubernetes/client-node'; -import { shellPromise } from '../shell'; -import { getLogger } from '../logger'; -import * as k8s from '@kubernetes/client-node'; import GlobalConfigService from '../../services/globalConfig'; import { createBuildJob } from '../kubernetes/jobFactory'; import { setupBuildServiceAccountInNamespace as setupServiceAccountWithRBAC } from '../kubernetes/rbac'; import { JobMonitor } from '../kubernetes/JobMonitor'; -export async function ensureNamespaceExists(namespace: string): Promise { - const kc = new k8s.KubeConfig(); - kc.loadFromDefault(); - const coreV1Api = kc.makeApiClient(k8s.CoreV1Api); - - try { - await coreV1Api.readNamespace(namespace); - getLogger().debug('Namespace: exists'); - } catch (error) { - if (error?.response?.statusCode === 404) { - getLogger().debug('Namespace: creating'); - await coreV1Api.createNamespace({ - metadata: { - name: namespace, - labels: { - 'app.kubernetes.io/managed-by': 'lifecycle', - }, - }, - }); - - await waitForNamespaceReady(namespace); - } else { - throw error; - } - } -} - -async function waitForNamespaceReady(namespace: string, timeout: number = 30000): Promise { - const startTime = Date.now(); - - while (Date.now() - startTime < timeout) { - try { - const result = await shellPromise(`kubectl get namespace ${namespace} -o jsonpath='{.status.phase}'`); - if (result.trim() === 'Active') { - return; - } - } catch (error) { - // Namespace not ready yet, will retry - } - - await new Promise((resolve) => setTimeout(resolve, 1000)); - } - - throw new Error(`Namespace ${namespace} did not become ready within ${timeout}ms`); -} - export async function setupBuildServiceAccountInNamespace( namespace: string, serviceAccountName: string = 'native-build-sa', diff --git a/src/server/services/__tests__/deploy.test.ts b/src/server/services/__tests__/deploy.test.ts index 26c48647..8034787b 100644 --- a/src/server/services/__tests__/deploy.test.ts +++ b/src/server/services/__tests__/deploy.test.ts @@ -32,7 +32,7 @@ const mockCodefreshWaitForImage = jest.fn(); const mockBuildWithNative = jest.fn(); const mockGlobalConfigGetAllConfigs = jest.fn(); const mockGlobalConfigGetOrgChartName = jest.fn(); -const mockEnsureNamespaceExists = jest.fn(); +const mockCreateOrUpdateNamespace = jest.fn(); jest.mock('server/lib/logger', () => ({ getLogger: jest.fn(() => ({ @@ -69,8 +69,8 @@ jest.mock('server/lib/nativeBuild', () => ({ buildWithNative: (...args: any[]) => mockBuildWithNative(...args), })); -jest.mock('server/lib/nativeBuild/utils', () => ({ - ensureNamespaceExists: (...args: any[]) => mockEnsureNamespaceExists(...args), +jest.mock('server/lib/kubernetes', () => ({ + createOrUpdateNamespace: (...args: any[]) => mockCreateOrUpdateNamespace(...args), })); const mockDetermineChartType = jest.fn(); @@ -124,7 +124,7 @@ describe('DeployService - shouldTriggerGithubDeployment', () => { mockCodefreshTagExists.mockReset(); mockCodefreshWaitForImage.mockReset(); mockBuildWithNative.mockReset(); - mockEnsureNamespaceExists.mockReset(); + mockCreateOrUpdateNamespace.mockReset(); mockGlobalConfigGetOrgChartName.mockResolvedValue('org-chart'); mockGlobalConfigGetAllConfigs.mockResolvedValue({ lifecycleDefaults: { @@ -422,6 +422,7 @@ describe('DeployService - shouldTriggerGithubDeployment', () => { uuid: 'sample-build', namespace: 'env-sample', enableFullYaml: true, + isStatic: false, commentRuntimeEnv: {}, enabledFeatures: [], pullRequest: { @@ -536,6 +537,7 @@ describe('DeployService - shouldTriggerGithubDeployment', () => { uuid: 'sample-build', namespace: 'env-sample', enableFullYaml: true, + isStatic: false, commentRuntimeEnv: {}, enabledFeatures: [], pullRequest: { @@ -571,7 +573,15 @@ describe('DeployService - shouldTriggerGithubDeployment', () => { expect(result).toBe(true); expect(mockBuildWithNative).not.toHaveBeenCalled(); - expect(mockEnsureNamespaceExists).toHaveBeenCalledWith('env-sample'); + expect(mockCreateOrUpdateNamespace).toHaveBeenCalledWith({ + name: 'env-sample', + buildUUID: 'sample-build', + staticEnv: false, + pullRequest: { + githubLogin: 'sample-user', + }, + waitForReady: true, + }); expect(processSecretsSpy).toHaveBeenCalledWith({ env: { API_TOKEN: '{{aws:repo/example-repo/api:API_TOKEN}}', diff --git a/src/server/services/build.ts b/src/server/services/build.ts index 7d78fb89..c9851147 100644 --- a/src/server/services/build.ts +++ b/src/server/services/build.ts @@ -1588,9 +1588,7 @@ export default class BuildService extends BaseService { name: build.namespace, buildUUID: build.uuid, staticEnv: build.isStatic, - repo: build.pullRequest?.fullName, - pullRequestNumber: build.pullRequest?.pullRequestNumber, - author: build.pullRequest?.githubLogin, + pullRequest: build.pullRequest, }); await k8s.createOrUpdateServiceAccount({ namespace: build.namespace, diff --git a/src/server/services/deploy.ts b/src/server/services/deploy.ts index 24a0a911..28263f0b 100644 --- a/src/server/services/deploy.ts +++ b/src/server/services/deploy.ts @@ -42,6 +42,7 @@ import { SecretProcessor } from 'server/services/secretProcessor'; import { fallbackDeployStatusMessage, statusMessageFromError } from 'server/lib/terminalFailure'; import { isNativeBuilderEngine } from 'server/lib/buildEngines'; import { SecretProvidersConfig } from 'server/services/types/globalConfig'; +import { createOrUpdateNamespace } from 'server/lib/kubernetes'; export interface DeployOptions { ownerId?: number; @@ -1034,8 +1035,15 @@ export default class DeployService extends BaseService { return emptyResult; } - const { ensureNamespaceExists } = await import('server/lib/nativeBuild/utils'); - await ensureNamespaceExists(deploy.build.namespace); + await deploy.$fetchGraph('[build.[pullRequest]]'); + + await createOrUpdateNamespace({ + name: deploy.build.namespace, + buildUUID: deploy.build.uuid, + staticEnv: deploy.build.isStatic, + pullRequest: deploy.build.pullRequest, + waitForReady: true, + }); const secretProcessor = new SecretProcessor(secretProviders); From fca576a3445900c77883655027cbc7073f135ec1 Mon Sep 17 00:00:00 2001 From: Vigneshraj Sekar Babu Date: Thu, 4 Jun 2026 17:10:36 -0700 Subject: [PATCH 2/2] refactor: share pull request label parsing --- src/server/lib/__tests__/utils.test.ts | 22 +++++++++++++++++++ src/server/lib/kubernetes.ts | 21 ++---------------- src/server/lib/utils.ts | 12 ++++++++++ .../services/__tests__/ttlCleanup.test.ts | 11 ++++++++++ src/server/services/ttlCleanup.ts | 17 +++++--------- 5 files changed, 53 insertions(+), 30 deletions(-) diff --git a/src/server/lib/__tests__/utils.test.ts b/src/server/lib/__tests__/utils.test.ts index 4e6d2ec4..6b6b532b 100644 --- a/src/server/lib/__tests__/utils.test.ts +++ b/src/server/lib/__tests__/utils.test.ts @@ -29,6 +29,7 @@ import { getStatusCommentLabel, isDefaultStatusCommentsEnabled, isControlCommentsEnabled, + parsePullRequestLabels, } from 'server/lib/utils'; import GlobalConfigService from 'server/services/globalConfig'; @@ -683,3 +684,24 @@ describe('isControlCommentsEnabled', () => { expect(result).toBe(true); }); }); + +describe('parsePullRequestLabels', () => { + test('returns an empty array for missing labels', () => { + expect(parsePullRequestLabels()).toEqual([]); + expect(parsePullRequestLabels(null)).toEqual([]); + }); + + test('returns array labels unchanged', () => { + const labels = ['deploy-env', 'keep-env']; + expect(parsePullRequestLabels(labels)).toBe(labels); + }); + + test('parses JSON string labels', () => { + expect(parsePullRequestLabels('["deploy-env","keep-env"]')).toEqual(['deploy-env', 'keep-env']); + }); + + test('returns an empty array for malformed or non-array JSON', () => { + expect(parsePullRequestLabels('not-json')).toEqual([]); + expect(parsePullRequestLabels('{"label":"deploy-env"}')).toEqual([]); + }); +}); diff --git a/src/server/lib/kubernetes.ts b/src/server/lib/kubernetes.ts index 07f4cfeb..11c9849f 100644 --- a/src/server/lib/kubernetes.ts +++ b/src/server/lib/kubernetes.ts @@ -20,7 +20,7 @@ import _ from 'lodash'; import { Build, Deploy, Deployable, Service } from 'server/models'; import { CLIDeployTypes, KubernetesDeployTypes, MEDIUM_TYPE, DEFAULT_TTL_INACTIVITY_DAYS } from 'shared/constants'; import { shellPromise } from './shell'; -import { flattenObject, getKeepLabel, waitUntil } from 'server/lib/utils'; +import { flattenObject, getKeepLabel, parsePullRequestLabels, waitUntil } from 'server/lib/utils'; import { ServiceDiskConfig } from 'server/models/yaml'; import * as k8s from '@kubernetes/client-node'; import { HttpError, V1Status, CoreV1Api, KubeConfig } from '@kubernetes/client-node'; @@ -152,23 +152,6 @@ function buildNamespacePrMetadata({ return { labels }; } -function parseNamespacePullRequestLabels(labels?: string[] | string | null): string[] { - if (!labels) { - return []; - } - - if (Array.isArray(labels)) { - return labels; - } - - try { - const parsed = JSON.parse(labels); - return Array.isArray(parsed) ? parsed : []; - } catch { - return []; - } -} - async function shouldEnableNamespaceTTL( staticEnv: boolean, pullRequest?: NamespacePullRequestMetadata | null @@ -178,7 +161,7 @@ async function shouldEnableNamespaceTTL( } const keepLabel = await getKeepLabel(); - return !parseNamespacePullRequestLabels(pullRequest?.labels).includes(keepLabel); + return !parsePullRequestLabels(pullRequest?.labels).includes(keepLabel); } /** diff --git a/src/server/lib/utils.ts b/src/server/lib/utils.ts index a6350f13..1554d31c 100644 --- a/src/server/lib/utils.ts +++ b/src/server/lib/utils.ts @@ -247,6 +247,18 @@ export const getKeepLabel = async (): Promise => { return labelsConfig?.keep?.[0] || FallbackLabels.KEEP; }; +export const parsePullRequestLabels = (labels?: string[] | string | null): string[] => { + if (!labels) return []; + if (Array.isArray(labels)) return labels; + + try { + const parsed = JSON.parse(labels); + return Array.isArray(parsed) ? parsed : []; + } catch { + return []; + } +}; + export const getStatusCommentLabel = async (): Promise => { const labelsConfig = await GlobalConfigService.getInstance().getLabels(); return labelsConfig?.statusComments?.[0] || FallbackLabels.STATUS_COMMENTS; diff --git a/src/server/services/__tests__/ttlCleanup.test.ts b/src/server/services/__tests__/ttlCleanup.test.ts index a769ba47..daf2ce45 100644 --- a/src/server/services/__tests__/ttlCleanup.test.ts +++ b/src/server/services/__tests__/ttlCleanup.test.ts @@ -67,6 +67,17 @@ jest.mock('server/lib/utils', () => ({ getKeepLabel: jest.fn(() => Promise.resolve('sample-keep')), getDisabledLabel: jest.fn(() => Promise.resolve('sample-disabled')), getDeployLabel: jest.fn(() => Promise.resolve('sample-deploy')), + parsePullRequestLabels: (labels?: string[] | string | null): string[] => { + if (!labels) return []; + if (Array.isArray(labels)) return labels; + + try { + const parsed = JSON.parse(labels); + return Array.isArray(parsed) ? parsed : []; + } catch { + return []; + } + }, })); jest.mock('server/lib/metrics', () => diff --git a/src/server/services/ttlCleanup.ts b/src/server/services/ttlCleanup.ts index 5283a890..d379c57f 100644 --- a/src/server/services/ttlCleanup.ts +++ b/src/server/services/ttlCleanup.ts @@ -21,7 +21,7 @@ import { redisClient } from 'server/lib/dependencies'; import { withLogContext, updateLogContext, getLogger, LogStage, extractContextForQueue } from 'server/lib/logger'; import * as k8s from '@kubernetes/client-node'; import { updatePullRequestLabels, createOrUpdatePullRequestComment, getPullRequestLabels } from 'server/lib/github'; -import { getKeepLabel, getDisabledLabel, getDeployLabel } from 'server/lib/utils'; +import { getKeepLabel, getDisabledLabel, getDeployLabel, parsePullRequestLabels } from 'server/lib/utils'; import { Build, PullRequest } from 'server/models'; import Metrics from 'server/lib/metrics'; import { DEFAULT_TTL_INACTIVITY_DAYS, DEFAULT_TTL_CHECK_INTERVAL_MINUTES, PullRequestStatus } from 'shared/constants'; @@ -114,11 +114,6 @@ export default class TTLCleanupService extends Service { }); }; - private parseLabels(labels: string | string[] | null): string[] { - if (!labels) return []; - return typeof labels === 'string' ? JSON.parse(labels) : labels; - } - private async getTTLConfig() { const globalConfig = await GlobalConfigService.getInstance().getAllConfigs(); @@ -237,7 +232,7 @@ export default class TTLCleanupService extends Service { build, pullRequest, daysExpired, - currentLabels: this.parseLabels(pullRequest.labels), + currentLabels: parsePullRequestLabels(pullRequest.labels), hadLabelDrift: false, }); continue; @@ -253,7 +248,7 @@ export default class TTLCleanupService extends Service { getLogger().debug(`Fetched ${currentLabels.length} labels from GitHub: ${currentLabels.join(', ')}`); - const dbLabels = this.parseLabels(pullRequest.labels); + const dbLabels = parsePullRequestLabels(pullRequest.labels); if (JSON.stringify(currentLabels.sort()) !== JSON.stringify(dbLabels.sort())) { getLogger().debug('TTL: label drift detected, syncing to DB'); await pullRequest.$query().patch({ @@ -262,7 +257,7 @@ export default class TTLCleanupService extends Service { } } catch (error) { getLogger().warn({ error }, 'TTL: GitHub labels fetch failed, using DB'); - currentLabels = this.parseLabels(pullRequest.labels); + currentLabels = parsePullRequestLabels(pullRequest.labels); } if (currentLabels.includes(keepLabel)) { @@ -275,7 +270,7 @@ export default class TTLCleanupService extends Service { continue; } - const dbLabels = this.parseLabels(pullRequest.labels); + const dbLabels = parsePullRequestLabels(pullRequest.labels); const hadLabelDrift = JSON.stringify(currentLabels.sort()) !== JSON.stringify(dbLabels.sort()); staleEnvironments.push({ @@ -354,7 +349,7 @@ export default class TTLCleanupService extends Service { const deployLabel = await getDeployLabel(); const disabledLabel = await getDisabledLabel(); - const currentLabels = this.parseLabels(pullRequest.labels); + const currentLabels = parsePullRequestLabels(pullRequest.labels); const updatedLabels = currentLabels.filter((label) => label !== deployLabel).concat(disabledLabel);