Merge pull request #8201 from divyanshiGupta/bug#1929577

Bug 1929577: Fix to avoid overriding of d/dc pod template container values
openshift · Feb 25, 2021 · 255395d · 255395d
2 parents 5770a70 + d6df541
commit 255395d
Show file tree

Hide file tree

Showing 8 changed files with 97 additions and 24 deletions.
diff --git a/frontend/packages/dev-console/src/components/edit-application/edit-application-utils.ts b/frontend/packages/dev-console/src/components/edit-application/edit-application-utils.ts
@@ -218,7 +218,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:

diff --git a/frontend/packages/dev-console/src/components/import/advanced/DeploymentConfigSection.tsx b/frontend/packages/dev-console/src/components/import/advanced/DeploymentConfigSection.tsx
@@ -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';
@@ -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
@@ -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']}
       />

diff --git a/frontend/packages/dev-console/src/components/import/deployImage-submit-utils.ts b/frontend/packages/dev-console/src/components/import/deployImage-submit-utils.ts
@@ -161,6 +161,7 @@ export const createOrUpdateDeployment = (
     ...annotations,
     'alpha.image.policy.openshift.io/resolve-names': '*',
     ...getTriggerAnnotation(
+      name,
       imgName || name,
       imgNamespace || namespace,
       imageChange,
@@ -440,6 +441,7 @@ export const createOrUpdateDeployImageResources = async (
     }
     const originalAnnotations = appResources?.editAppResource?.data?.metadata?.annotations || {};
     const triggerAnnotations = getTriggerAnnotation(
+      name,
       internalImageStreamName || name,
       internalImageStreamNamespace || namespace,
       imageChange,

diff --git a/frontend/packages/dev-console/src/components/import/import-submit-utils.ts b/frontend/packages/dev-console/src/components/import/import-submit-utils.ts
@@ -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);
@@ -632,6 +633,7 @@ export const createOrUpdateResources = async (
 
     const originalAnnotations = appResources?.editAppResource?.data?.metadata?.annotations || {};
     const triggerAnnotations = getTriggerAnnotation(
+      name,
       generatedImageStreamName || name,
       namespace,
       imageChange,

diff --git a/frontend/packages/dev-console/src/utils/__tests__/resource-label-utils-data.ts b/frontend/packages/dev-console/src/utils/__tests__/resource-label-utils-data.ts
@@ -55,6 +55,13 @@ export const originalDeployment = {
                 protocol: 'TCP',
               },
             ],
+            envFrom: [
+              {
+                configMapRef: {
+                  name: 'testconfig',
+                },
+              },
+            ],
             volumeMounts: [
               {
                 name: 'test-volume',

diff --git a/frontend/packages/dev-console/src/utils/__tests__/resource-label-utils.spec.tsx b/frontend/packages/dev-console/src/utils/__tests__/resource-label-utils.spec.tsx
@@ -1,4 +1,4 @@
-import { mergeData } from '../resource-label-utils';
+import { getTriggerAnnotation, mergeData } from '../resource-label-utils';
 import {
   devfileDeployment,
   newBuildConfig,
@@ -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', () => {
@@ -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"}]',
+      });
     });
   });
 });
diff --git a/frontend/packages/dev-console/src/utils/resource-label-utils.ts b/frontend/packages/dev-console/src/utils/resource-label-utils.ts
@@ -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}`,
     },
   ]),
@@ -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;
@@ -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;
 };

diff --git a/frontend/packages/knative-plugin/src/utils/create-knative-utils.ts b/frontend/packages/knative-plugin/src/utils/create-knative-utils.ts
@@ -106,6 +106,7 @@ export const getKnativeServiceDepResource = (
           ...(concurrencylimit && { containerConcurrency: concurrencylimit }),
           containers: [
             {
+              name,
               image: `${imageStreamUrl}`,
               ...(contTargetPort && {
                 ports: [