Skip to content

Commit

Permalink
Merge pull request #2849 from andrewballantyne/update-jupyter-cert-impl
Browse files Browse the repository at this point in the history
Merge smarter jupyter values
  • Loading branch information
openshift-merge-bot[bot] authored May 23, 2024
2 parents 3bf5da1 + c5e1bce commit 28efbe4
Show file tree
Hide file tree
Showing 4 changed files with 122 additions and 4 deletions.
69 changes: 69 additions & 0 deletions backend/src/__tests__/objUtils.spec.ts
Original file line number Diff line number Diff line change
@@ -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' },
]);
});
});
});
7 changes: 6 additions & 1 deletion backend/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -766,7 +766,12 @@ export type DetectedAccelerators = {

export type EnvironmentVariable = EitherNotBoth<
{ value: string | number },
{ valueFrom: Record<string, unknown> }
{
valueFrom: Record<string, unknown> & {
configMapKeyRef?: { key: string; name: string };
secretKeyRef?: { key: string; name: string };
};
}
> & {
name: string;
};
Expand Down
20 changes: 17 additions & 3 deletions backend/src/utils/notebookUtils.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { getDashboardConfig } from './resourceUtils';
import { merge } from 'lodash';
import { mergeWith } from 'lodash';
import {
ContainerResourceAttributes,
EnvironmentVariable,
Expand Down Expand Up @@ -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 =>
Expand Down Expand Up @@ -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,
},
],
Expand All @@ -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',
Expand Down
30 changes: 30 additions & 0 deletions backend/src/utils/objUtils.ts
Original file line number Diff line number Diff line change
@@ -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<GoodArray>((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);
}
};

0 comments on commit 28efbe4

Please sign in to comment.