From c5e1bce5ad8be243a7a9c00e0b2325d7dccff1a1 Mon Sep 17 00:00:00 2001 From: Andrew Ballantyne Date: Fri, 10 May 2024 13:03:30 -0400 Subject: [PATCH] Merge smarter jupyter values --- backend/src/__tests__/objUtils.spec.ts | 69 ++++++++++++++++++++++++++ backend/src/types.ts | 7 ++- backend/src/utils/notebookUtils.ts | 20 ++++++-- backend/src/utils/objUtils.ts | 30 +++++++++++ 4 files changed, 122 insertions(+), 4 deletions(-) create mode 100644 backend/src/__tests__/objUtils.spec.ts create mode 100644 backend/src/utils/objUtils.ts diff --git a/backend/src/__tests__/objUtils.spec.ts b/backend/src/__tests__/objUtils.spec.ts new file mode 100644 index 0000000000..6915693a3b --- /dev/null +++ b/backend/src/__tests__/objUtils.spec.ts @@ -0,0 +1,69 @@ +import { smartMergeArraysWithNameObjects } from '../utils/objUtils'; + +describe('objUtils', () => { + describe('smartMergeArraysWithNameObjects', () => { + /** we only use the first two params of the customization utility, so ignore the last 3 */ + const smartMergeArraysWithNameObjectsWithUsedParams = (v1: any, v2: any) => + smartMergeArraysWithNameObjects(v1, v2, undefined, undefined, undefined); + + it('should do nothing with nulls', () => { + expect(smartMergeArraysWithNameObjectsWithUsedParams(null, null)).toBe(undefined); + }); + + it('should do nothing with objects', () => { + expect( + smartMergeArraysWithNameObjectsWithUsedParams({ a: true }, { a: false, b: true }), + ).toBe(undefined); + }); + + it('should do nothing with string[]', () => { + expect(smartMergeArraysWithNameObjectsWithUsedParams(['test'], ['test2'])).toBe(undefined); + }); + + it('should do nothing with invalid object arrays', () => { + expect( + smartMergeArraysWithNameObjectsWithUsedParams([{ id: 'test' }], [{ id: 'test2' }]), + ).toBe(undefined); + }); + + it('should replace 2nd object if given two same correct objects arrays', () => { + expect( + smartMergeArraysWithNameObjectsWithUsedParams( + [{ name: 'test', value: '1' }], + [{ name: 'test', value: '2' }], + ), + ).toEqual([{ name: 'test', value: '2' }]); + }); + + it('should add 2nd object if given two different correct object arrays', () => { + expect( + smartMergeArraysWithNameObjectsWithUsedParams( + [{ name: 'test', value: '1' }], + [{ name: 'test2', value: '2' }], + ), + ).toEqual([ + { name: 'test', value: '1' }, + { name: 'test2', value: '2' }, + ]); + }); + + it('should replace and add as appropriate if given two correct object arrays', () => { + expect( + smartMergeArraysWithNameObjectsWithUsedParams( + [ + { name: 'test', value: '1' }, + { name: 'test3', value: '3' }, + ], + [ + { name: 'test', value: '1b' }, + { name: 'test2', value: '2' }, + ], + ), + ).toEqual([ + { name: 'test', value: '1b' }, + { name: 'test3', value: '3' }, + { name: 'test2', value: '2' }, + ]); + }); + }); +}); diff --git a/backend/src/types.ts b/backend/src/types.ts index bde7b3a527..a65214c6d4 100644 --- a/backend/src/types.ts +++ b/backend/src/types.ts @@ -766,7 +766,12 @@ export type DetectedAccelerators = { export type EnvironmentVariable = EitherNotBoth< { value: string | number }, - { valueFrom: Record } + { + valueFrom: Record & { + configMapKeyRef?: { key: string; name: string }; + secretKeyRef?: { key: string; name: string }; + }; + } > & { name: string; }; diff --git a/backend/src/utils/notebookUtils.ts b/backend/src/utils/notebookUtils.ts index adaf935b59..319d4851d9 100644 --- a/backend/src/utils/notebookUtils.ts +++ b/backend/src/utils/notebookUtils.ts @@ -1,5 +1,5 @@ import { getDashboardConfig } from './resourceUtils'; -import { merge } from 'lodash'; +import { mergeWith } from 'lodash'; import { ContainerResourceAttributes, EnvironmentVariable, @@ -31,6 +31,7 @@ import { import { DEFAULT_NOTEBOOK_SIZES, DEFAULT_PVC_SIZE, MOUNT_PATH } from './constants'; import { FastifyRequest } from 'fastify'; import { verifyEnvVars } from './envUtils'; +import { smartMergeArraysWithNameObjects } from './objUtils'; import { getImageInfo } from '../routes/api/images/imageUtils'; export const generateNotebookNameFromUsername = (username: string): string => @@ -534,7 +535,15 @@ export const updateNotebook = async ( spec: { containers: [ { - env: oldNotebook.spec.template.spec.containers[0].env, + // Drop all env vars we added in the past, because we will just add them back if they are still there + env: oldNotebook.spec.template.spec.containers[0].env.filter(({ valueFrom }) => { + if (!valueFrom) { + return true; + } else { + const value = valueFrom.secretKeyRef ?? valueFrom.configMapKeyRef; + return !value?.name?.startsWith('jupyterhub-singleuser-profile'); + } + }), volumeMounts: oldNotebook.spec.template.spec.containers[0].volumeMounts, }, ], @@ -544,7 +553,12 @@ export const updateNotebook = async ( }, }; - const notebookAssembled = merge({}, importantOldNotebookDetails, serverNotebook); + const notebookAssembled = mergeWith( + {}, + importantOldNotebookDetails, + serverNotebook, + smartMergeArraysWithNameObjects, + ); const response = await fastify.kube.customObjectsApi.patchNamespacedCustomObject( 'kubeflow.org', diff --git a/backend/src/utils/objUtils.ts b/backend/src/utils/objUtils.ts new file mode 100644 index 0000000000..db78e8381b --- /dev/null +++ b/backend/src/utils/objUtils.ts @@ -0,0 +1,30 @@ +import { MergeWithCustomizer } from 'lodash'; + +/** + * When given two object arrays that have "name" keys, replace when keys are the same, or add to + * the end when new key. + * + * Note: returns `undefined` if invalid data is provided. + * + * @see mergeWith -- lodash mergeWith customizer + */ +export const smartMergeArraysWithNameObjects: MergeWithCustomizer = (objValue, srcValue) => { + type GoodArray = { name: string }[]; + const isGoodArray = (v: any): v is GoodArray => Array.isArray(v) && v.length > 0 && v[0].name; + if (isGoodArray(objValue) && isGoodArray(srcValue)) { + // Arrays with objects that have a .name property, sync on merge for the name + return srcValue.reduce((newArray, elm) => { + const key = elm.name; + const index = newArray.findIndex(({ name }) => name === key); + if (index >= 0) { + // existing value, replace + newArray[index] = elm; + } else { + // didn't find existing name, add to end + newArray.push(elm); + } + + return newArray; + }, objValue); + } +};