Sync data between Create Operand YAML and Form #3894

TheRealJon · 2020-01-08T15:20:26Z

Sync data between yaml and form editors in the create operand workflow. Also fix visual issue where field group names overlap left blue border. It appears that the implementation of the blue border on patternfly accordions changed at some point and our css to override those styles was no longer working as expected.

Also update olm descriptor integration tests to delete the test operand between testing list/details views and creation form. There was previously no cleanup between these tests, which was causing a failure when attempting to create a new operand during the latter steps.

TheRealJon · 2020-01-08T19:21:51Z

/retest

TheRealJon · 2020-01-23T15:54:00Z

@benjaminapetersen @spadgett This is ready for review.

spadgett

/approve
/lgtm

Nice work on this. It would be good to switch to optional chaining while changing the code, but since this was in existing code, I don't want to hold up the PR.

spadgett · 2020-01-23T16:15:58Z

frontend/packages/operator-lifecycle-manager/src/components/create-operand.tsx

-            ],
+        .map((field) => {
+          const capabilities = field.capabilities || [];
+          const openAPIProperties = _.get(openAPI, 'properties.spec.properties');


Suggested change

const openAPIProperties = _.get(openAPI, 'properties.spec.properties');

const openAPIProperties = openAPI?.properties?.spec?.properties;

spadgett · 2020-01-23T16:16:37Z

frontend/packages/operator-lifecycle-manager/src/components/create-operand.tsx


  const [formValues, setFormValues] = React.useState<FormValues>({
-    'metadata.name': 'example',
-    'metadata.labels': [],
+    'metadata.name': _.get(sample, 'metadata.name', 'example'),


Suggested change

'metadata.name': _.get(sample, 'metadata.name', 'example'),

'metadata.name': sample?.metadata?.name || 'example',

spadgett · 2020-01-23T16:17:10Z

frontend/packages/operator-lifecycle-manager/src/components/create-operand.tsx

-    'metadata.name': 'example',
-    'metadata.labels': [],
+    'metadata.name': _.get(sample, 'metadata.name', 'example'),
+    'metadata.labels': SelectorInput.arrayify(_.get(sample, 'metadata.labels')) || [],


Suggested change

'metadata.labels': SelectorInput.arrayify(_.get(sample, 'metadata.labels')) || [],

'metadata.labels': SelectorInput.arrayify(sample?.metadata?.labels) || [],

spadgett · 2020-01-23T16:18:36Z

frontend/packages/operator-lifecycle-manager/src/components/create-operand.tsx

        },
        spec: _.reduce(
          specValues,
          (spec, value, path) => _.set(spec, path, value),
-          _.get(props.sample, 'spec', {}),
+          _.get(sample, 'spec', {}),


Suggested change

_.get(sample, 'spec', {}),

sample?.spec || {},

spadgett · 2020-01-23T16:18:51Z

frontend/packages/operator-lifecycle-manager/src/components/create-operand.tsx

          name: formValues['metadata.name'],
          labels: SelectorInput.objectify(
            formValues['metadata.labels'],
          ) as ObjectMetadata['labels'],
-          annotations: _.get(props.sample, 'metadata.annotations', {}),
+          annotations: _.get(sample, 'metadata.annotations', {}),


Suggested change

annotations: _.get(sample, 'metadata.annotations', {}),

annotations: sample?.metadata?.annotations || {},

spadgett · 2020-01-23T16:20:30Z

frontend/packages/operator-lifecycle-manager/src/components/create-operand.tsx

-                icon={_.get(props.clusterServiceVersion, 'spec.icon[0]')}
-                provider={_.get(props.clusterServiceVersion, 'spec.provider')}
+                displayName={providedAPI.displayName}
+                icon={_.get(clusterServiceVersion, 'spec.icon[0]')}


Suggested change

icon={_.get(clusterServiceVersion, 'spec.icon[0]')}

icon={clusterServiceVersion?.spec?.icon?.[0]}

spadgett · 2020-01-23T16:20:46Z

frontend/packages/operator-lifecycle-manager/src/components/create-operand.tsx

-                provider={_.get(props.clusterServiceVersion, 'spec.provider')}
+                displayName={providedAPI.displayName}
+                icon={_.get(clusterServiceVersion, 'spec.icon[0]')}
+                provider={_.get(clusterServiceVersion, 'spec.provider')}


Suggested change

provider={_.get(clusterServiceVersion, 'spec.provider')}

provider={clusterServiceVersion?.spec?.provider}

spadgett · 2020-01-23T16:21:19Z

frontend/packages/operator-lifecycle-manager/src/components/create-operand.tsx

+  operandModel,
+}) => {
+  const { data: csv } = clusterServiceVersion;
+  const csvAnnotations = _.get(csv, 'metadata.annotations', {});


Suggested change

const csvAnnotations = _.get(csv, 'metadata.annotations', {});

const csvAnnotations = csv?.metadata?.annotations || {};

spadgett · 2020-01-23T16:21:59Z

frontend/packages/operator-lifecycle-manager/src/components/create-operand.tsx

+
+  const defaultSample = React.useMemo<K8sResourceKind>(
+    () =>
+      JSON.parse(_.get(csvAnnotations, annotationKey, '[]')).find(


Suggested change

JSON.parse(_.get(csvAnnotations, annotationKey, '[]')).find(

JSON.parse(csvAnnotations?.[annotationKey] || '[]').find(

TheRealJon · 2020-01-23T16:37:26Z

Nice work on this. It would be good to switch to optional chaining while changing the code, but since this was in existing code, I don't want to hold up the PR.

@spadgett I did consider converting to optional chaining in a lot of those places. Decided against it to keep my changes to a minimum. Clearly worked out in my favor with the size of this PR 😆

openshift-bot · 2020-01-23T18:10:58Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-23T18:26:57Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-23T20:58:56Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-23T21:11:58Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-23T22:30:04Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-23T23:50:50Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-24T00:17:19Z

/retest

Please review the full test history for this PR and help us cut down flakes.

spadgett · 2020-01-24T00:23:46Z

/lgtm cancel

Runtime error in crud scenario

[SEVERE] https://console-openshift-console.apps.ci-op-hl469dws-75d12.origin-ci-int-gce.dev.openshift.com/static/vendors~main-chunk-f13f313b764870d9c578.min.js 117:71742 TypeError: l is not a function
    at t.<anonymous> (https://console-openshift-console.apps.ci-op-hl469dws-75d12.origin-ci-int-gce.dev.openshift.com/static/12-chunk-2b438f585f40935147f6.min.js:1:16172)
    at Ro (https://console-openshift-console.apps.ci-op-hl469dws-75d12.origin-ci-int-gce.dev.openshift.com/static/vendors~main-chunk-f13f313b764870d9c578.min.js:118:45996)
    at Ho (https://console-openshift-console.apps.ci-op-hl469dws-75d12.origin-ci-int-gce.dev.openshift.com/static/vendors~main-chunk-f13f313b764870d9c578.min.js:118:45733)
    at https://console-openshift-console.apps.ci-op-hl469dws-75d12.origin-ci-int-gce.dev.openshift.com/static/vendors~main-chunk-f13f313b764870d9c578.min.js:118:95064
    at e.unstable_runWithPriority (https://console-openshift-console.apps.ci-op-hl469dws-75d12.origin-ci-int-gce.dev.openshift.com/static/vendors~main-chunk-f13f313b764870d9c578.min.js:126:3878)
    at so (https://console-openshift-console.apps.ci-op-hl469dws-75d12.origin-ci-int-gce.dev.openshift.com/static/vendors~main-chunk-f13f313b764870d9c578.min.js:118:41488)
    at Bc (https://console-openshift-console.apps.ci-op-hl469dws-75d12.origin-ci-int-gce.dev.openshift.com/static/vendors~main-chunk-f13f313b764870d9c578.min.js:118:90936)
    at Tc (https://console-openshift-console.apps.ci-op-hl469dws-75d12.origin-ci-int-gce.dev.openshift.com/static/vendors~main-chunk-f13f313b764870d9c578.min.js:118:79608)
    at https://console-openshift-console.apps.ci-op-hl469dws-75d12.origin-ci-int-gce.dev.openshift.com/static/vendors~main-chunk-f13f313b764870d9c578.min.js:118:41759
    at e.unstable_runWithPriority (https://console-openshift-console.apps.ci-op-hl469dws-75d12.origin-ci-int-gce.dev.openshift.com/static/vendors~main-chunk-f13f313b764870d9c578.min.js:126:3878)

spadgett · 2020-01-27T21:23:52Z

/hold cancel

TheRealJon · 2020-01-27T21:26:20Z

@spadgett It looks like the olm test was the problem. Looking at https://github.com/openshift/console/blob/006516745042a2d99f65f70ca9eded19a0600f9a/frontend/packages/operator-lifecycle-manager/integration-tests/scenarios/descriptors.scenario.ts#L348-L350p

A new operand is created at the beginning of that test scenario but is not cleaned up until after all of the test cases run. We check the list and details pages, then move on to the operand creation form, where we try to create a new operand using the form. This step was failing because it is trying to create a duplicate operand. Not sure why or how we weren't failing on this before now.

TheRealJon · 2020-01-27T21:53:52Z

frontend/packages/operator-lifecycle-manager/src/components/create-operand.tsx

@@ -314,98 +335,114 @@ export const CreateOperandForm: React.FC<CreateOperandFormProps> = (props) => {
  };

  const [formValues, setFormValues] = React.useState<FormValues>({
-    'metadata.name': 'example',
-    'metadata.labels': [],
+    'metadata.name': _.get(sample, 'metadata.name', 'example'),


This line is what was causing the integration test failure. Previously, metadata.name was not synced from the YAML view and just defaulted to 'example', so when the integration tests created a new operand using the form, it didn't conflict with the existing one. Now that data is synced, the metadata.name field from the YAML view is carried over to the form view and conflicts with the existing resource.

@benjaminapetersen @spadgett I guess an easier solution to this failure would be to add a step to change the operand name before we create it via the form. But... since deleting and recreating it seems to work just fine (and adds a little bit more coverage), we might as well just leave it. Any objections or thoughts?

benjaminapetersen · 2020-01-28T14:40:27Z

/retest

benjaminapetersen · 2020-01-28T14:42:14Z

/hold

nevermind, CI is broken.

spadgett · 2020-01-28T16:39:37Z

/lgtm
/hold cancel

openshift-ci-robot · 2020-01-28T16:41:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benjaminapetersen, spadgett, TheRealJon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [benjaminapetersen,spadgett]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

TheRealJon · 2020-01-28T18:52:29Z

/retest

TheRealJon · 2020-01-28T20:37:57Z

/retest

TheRealJon · 2020-01-28T20:58:01Z

/retest

TheRealJon · 2020-01-28T22:24:49Z

/retest

TheRealJon · 2020-01-29T01:59:54Z

/retest

TheRealJon · 2020-01-29T04:13:50Z

/retest

openshift-bot · 2020-01-29T09:22:10Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-29T09:48:20Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-29T11:45:51Z

/retest

Please review the full test history for this PR and help us cut down flakes.

TheRealJon · 2020-01-29T13:49:44Z

/retest

openshift-bot · 2020-01-29T14:20:13Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-29T15:51:29Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-01-29T18:14:10Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 8, 2020

openshift-ci-robot requested review from jeff-phillips-18 and zherman0 January 8, 2020 15:21

openshift-ci-robot added component/core Related to console core functionality component/olm Related to OLM labels Jan 8, 2020

TheRealJon force-pushed the 1965-console branch from fd7f449 to 771a446 Compare January 8, 2020 17:35

TheRealJon force-pushed the 1965-console branch from 771a446 to 7137373 Compare January 23, 2020 15:46

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 23, 2020

TheRealJon changed the title ~~[WIP] Sync data between Create Operand YAML and Form~~ Sync data between Create Operand YAML and Form Jan 23, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 23, 2020

spadgett approved these changes Jan 23, 2020

View reviewed changes

openshift-ci-robot assigned spadgett Jan 23, 2020

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 23, 2020

openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 24, 2020

TheRealJon force-pushed the 1965-console branch from 7137373 to 5114672 Compare January 24, 2020 16:46

Sync data between create Operand YAML and Form

3dd119f

TheRealJon force-pushed the 1965-console branch from 95db8cb to 3dd119f Compare January 27, 2020 21:21

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 27, 2020

TheRealJon commented Jan 27, 2020

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 28, 2020

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 28, 2020

openshift-merge-robot merged commit 72b67ed into openshift:master Jan 29, 2020

spadgett added this to the v4.4 milestone Feb 4, 2020

spadgett mentioned this pull request May 11, 2020

Data lost between CR YAML and Form switch #5225

Closed

TheRealJon deleted the 1965-console branch January 28, 2022 14:26

	const openAPIProperties = _.get(openAPI, 'properties.spec.properties');
	const openAPIProperties = openAPI?.properties?.spec?.properties;

	'metadata.name': _.get(sample, 'metadata.name', 'example'),
	'metadata.name': sample?.metadata?.name \|\| 'example',

	'metadata.labels': SelectorInput.arrayify(_.get(sample, 'metadata.labels')) \|\| [],
	'metadata.labels': SelectorInput.arrayify(sample?.metadata?.labels) \|\| [],

	annotations: _.get(sample, 'metadata.annotations', {}),
	annotations: sample?.metadata?.annotations \|\| {},

	icon={_.get(clusterServiceVersion, 'spec.icon[0]')}
	icon={clusterServiceVersion?.spec?.icon?.[0]}

	provider={_.get(clusterServiceVersion, 'spec.provider')}
	provider={clusterServiceVersion?.spec?.provider}

	const csvAnnotations = _.get(csv, 'metadata.annotations', {});
	const csvAnnotations = csv?.metadata?.annotations \|\| {};

	JSON.parse(_.get(csvAnnotations, annotationKey, '[]')).find(
	JSON.parse(csvAnnotations?.[annotationKey] \|\| '[]').find(

Sync data between Create Operand YAML and Form #3894

Sync data between Create Operand YAML and Form #3894

Conversation

TheRealJon commented Jan 8, 2020 • edited

TheRealJon commented Jan 8, 2020

TheRealJon commented Jan 23, 2020

spadgett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealJon commented Jan 23, 2020

openshift-bot commented Jan 23, 2020

openshift-bot commented Jan 23, 2020

openshift-bot commented Jan 23, 2020

openshift-bot commented Jan 23, 2020

openshift-bot commented Jan 23, 2020

openshift-bot commented Jan 23, 2020

openshift-bot commented Jan 24, 2020

spadgett commented Jan 24, 2020

spadgett commented Jan 27, 2020

TheRealJon commented Jan 27, 2020

TheRealJon Jan 27, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminapetersen commented Jan 28, 2020

benjaminapetersen commented Jan 28, 2020

spadgett commented Jan 28, 2020

openshift-ci-robot commented Jan 28, 2020

TheRealJon commented Jan 28, 2020

TheRealJon commented Jan 28, 2020

TheRealJon commented Jan 28, 2020

TheRealJon commented Jan 28, 2020

TheRealJon commented Jan 29, 2020

TheRealJon commented Jan 29, 2020

openshift-bot commented Jan 29, 2020

openshift-bot commented Jan 29, 2020

openshift-bot commented Jan 29, 2020

TheRealJon commented Jan 29, 2020

openshift-bot commented Jan 29, 2020

openshift-bot commented Jan 29, 2020

openshift-bot commented Jan 29, 2020

TheRealJon commented Jan 8, 2020 •

edited

TheRealJon Jan 27, 2020 •

edited