Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 1929577: Fix to avoid overriding of d/dc pod template container values #8201

Merged
merged 2 commits into from
Feb 25, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,10 @@ export const getDeploymentData = (resource: K8sResourceKind) => {
replicas: 1,
triggers: { image: true, config: true },
};
const container = resource.spec?.template?.spec?.containers?.[0];
const container = _.find(
resource.spec?.template?.spec?.containers,
(c) => c.name === resource.metadata.name,
);
const env = container?.env ?? [];
switch (getResourcesType(resource)) {
case Resources.KnativeService:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import * as React from 'react';
import * as _ from 'lodash';
import { useTranslation } from 'react-i18next';
import { CheckboxField, EnvironmentField } from '@console/shared';
import { K8sResourceKind } from '@console/internal/module/k8s';
Expand All @@ -18,15 +17,18 @@ const DeploymentConfigSection: React.FC<DeploymentConfigSectionProps> = ({
}) => {
const { t } = useTranslation();
const {
values: { resources },
values: {
resources,
deployment: { env },
},
} = useFormikContext<FormikValues>();
const deploymentConfigObj = resource || {
kind: 'DeploymentConfig',
metadata: {
namespace,
},
};
const envs = _.get(deploymentConfigObj, 'spec.template.spec.containers[0].env', []);

return (
<FormSection title={t('devconsole~Deployment')} fullWidth>
<CheckboxField
Expand All @@ -42,7 +44,7 @@ const DeploymentConfigSection: React.FC<DeploymentConfigSectionProps> = ({
<EnvironmentField
name="deployment.env"
label={t('devconsole~Environment variables (runtime only)')}
envs={envs}
envs={env}
obj={deploymentConfigObj}
envPath={['spec', 'template', 'spec', 'containers']}
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ export const createOrUpdateDeployment = (
...annotations,
'alpha.image.policy.openshift.io/resolve-names': '*',
...getTriggerAnnotation(
name,
imgName || name,
imgNamespace || namespace,
imageChange,
Expand Down Expand Up @@ -440,6 +441,7 @@ export const createOrUpdateDeployImageResources = async (
}
const originalAnnotations = appResources?.editAppResource?.data?.metadata?.annotations || {};
const triggerAnnotations = getTriggerAnnotation(
name,
internalImageStreamName || name,
internalImageStreamNamespace || namespace,
imageChange,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -265,11 +265,12 @@ export const createOrUpdateDeployment = (

const imageStreamName = imageStream && imageStream.metadata.name;
const defaultLabels = getAppLabels({ name, applicationName, imageStreamName, selectedTag });
const imageName = name;
const annotations = {
...getGitAnnotations(repository, ref),
...getCommonAnnotations(),
'alpha.image.policy.openshift.io/resolve-names': '*',
...getTriggerAnnotation(name, namespace, imageChange),
...getTriggerAnnotation(name, imageName, namespace, imageChange),
};
const podLabels = getPodLabels(name);
const templateLabels = getTemplateLabels(originalDeployment);
Expand Down Expand Up @@ -632,6 +633,7 @@ export const createOrUpdateResources = async (

const originalAnnotations = appResources?.editAppResource?.data?.metadata?.annotations || {};
const triggerAnnotations = getTriggerAnnotation(
name,
generatedImageStreamName || name,
namespace,
imageChange,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,13 @@ export const originalDeployment = {
protocol: 'TCP',
},
],
envFrom: [
{
configMapRef: {
name: 'testconfig',
},
},
],
volumeMounts: [
{
name: 'test-volume',
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { mergeData } from '../resource-label-utils';
import { getTriggerAnnotation, mergeData } from '../resource-label-utils';
import {
devfileDeployment,
newBuildConfig,
Expand Down Expand Up @@ -49,11 +49,44 @@ describe('resource-label-utils', () => {
);
});

it('should return mergedData with newResource template containers', () => {
it('should return mergedData with newResource template container values', () => {
const mergedResource = mergeData(originalDeployment, newDeployment);
expect(mergedResource.spec.template.spec.containers).toEqual(
newDeployment.spec.template.spec.containers,
);
expect(mergedResource.spec.template.spec.containers).toEqual([
{
name: 'nationalparks-py',
image:
'image-registry.openshift-image-registry.svc:5000/div/nationalparks-py@sha256:8b187a8f235f42e7ea3e21e740c4940fdfa3ec8b59a14bb1cd9a67ffedf2eef9',
ports: [
{
containerPort: 8080,
protocol: 'TCP',
},
],
env: [
{
name: 'dev',
value: 'test',
},
],
envFrom: [
{
configMapRef: {
name: 'testconfig',
},
},
],
volumeMounts: [
{
name: 'test-volume',
mountPath: '/test',
},
],
resources: {},
terminationMessagePath: '/dev/termination-log',
terminationMessagePolicy: 'File',
imagePullPolicy: 'Always',
},
]);
});

it('should return mergedData with newResource strategy', () => {
Expand All @@ -71,14 +104,25 @@ describe('resource-label-utils', () => {
expect(mergedResource.spec.triggers).toEqual(newDeploymentConfig.spec.triggers);
});

it('should return mergedData with originalResource template volumes and volumeMounts ', () => {
it('should return mergedData with originalResource template volumes', () => {
const mergedResource = mergeData(originalDeployment, newDeployment);
expect(mergedResource.spec.template.spec.volumes).toEqual(
originalDeployment.spec.template.spec.volumes,
);
expect(mergedResource.spec.template.spec.containers[0].volumeMounts).toEqual(
originalDeployment.spec.template.spec.containers[0].volumeMounts,
);
});
});
describe('getTriggerAnnotation', () => {
it('should return trigger annotation with proper values', () => {
let annotation = getTriggerAnnotation('test', 'python', 'div', true);
expect(annotation).toEqual({
'image.openshift.io/triggers':
'[{"from":{"kind":"ImageStreamTag","name":"python:latest","namespace":"div"},"fieldPath":"spec.template.spec.containers[?(@.name==\\"test\\")].image","pause":"false"}]',
});
annotation = getTriggerAnnotation('test', 'test', 'div', false);
expect(annotation).toEqual({
'image.openshift.io/triggers':
'[{"from":{"kind":"ImageStreamTag","name":"test:latest","namespace":"div"},"fieldPath":"spec.template.spec.containers[?(@.name==\\"test\\")].image","pause":"true"}]',
});
});
});
});
30 changes: 21 additions & 9 deletions frontend/packages/dev-console/src/utils/resource-label-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,15 +57,16 @@ export const getCommonAnnotations = () => {
};

export const getTriggerAnnotation = (
name: string,
namespace: string,
containerName: string,
imageName: string,
imageNamespace: string,
imageTrigger: boolean,
tag: string = 'latest',
imageTag: string = 'latest',
) => ({
[TRIGGERS_ANNOTATION]: JSON.stringify([
{
from: { kind: 'ImageStreamTag', name: `${name}:${tag}`, namespace },
fieldPath: `spec.template.spec.containers[?(@.name=="${name}")].image`,
from: { kind: 'ImageStreamTag', name: `${imageName}:${imageTag}`, namespace: imageNamespace },
fieldPath: `spec.template.spec.containers[?(@.name=="${containerName}")].image`,
pause: `${!imageTrigger}`,
},
]),
Expand Down Expand Up @@ -96,8 +97,21 @@ export const mergeData = (originalResource: K8sResourceKind, newResource: K8sRes
if (mergedData.spec?.template?.metadata?.labels) {
mergedData.spec.template.metadata.labels = newResource.spec?.template?.metadata?.labels;
}
if (mergedData.spec?.template?.spec?.containers) {
mergedData.spec.template.spec.containers = newResource.spec.template.spec.containers;
if (!_.isEmpty(originalResource.spec?.template?.spec?.containers)) {
mergedData.spec.template.spec.containers = originalResource.spec.template.spec.containers;
const index = _.findIndex(originalResource.spec.template.spec.containers, {
name: originalResource.metadata.name,
});
if (index >= 0) {
mergedData.spec.template.spec.containers[index] = {
...originalResource.spec.template.spec.containers[index],
...newResource.spec.template.spec.containers[0],
// Keep the volumeMounts as is since we do not give an option to edit these currently
volumeMounts: originalResource.spec.template.spec.containers[index].volumeMounts,
};
} else {
mergedData.spec.template.spec.containers.push(newResource.spec.template.spec.containers[0]);
}
}
if (mergedData?.spec?.hasOwnProperty('strategy')) {
mergedData.spec.strategy = newResource.spec?.strategy ?? originalResource.spec?.strategy;
Expand All @@ -107,8 +121,6 @@ export const mergeData = (originalResource: K8sResourceKind, newResource: K8sRes
}
if (mergedData.spec?.template?.spec?.hasOwnProperty('volumes')) {
mergedData.spec.template.spec.volumes = originalResource.spec?.template?.spec?.volumes;
mergedData.spec.template.spec.containers[0].volumeMounts =
originalResource.spec?.template?.spec?.containers?.[0]?.volumeMounts;
}
return mergedData;
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ export const getKnativeServiceDepResource = (
...(concurrencylimit && { containerConcurrency: concurrencylimit }),
containers: [
{
name,
image: `${imageStreamUrl}`,
...(contTargetPort && {
ports: [
Expand Down