Bug 1753812: modify view update strategy descriptor #2864

cajieh · 2019-09-27T20:43:26Z

cajieh · 2019-09-27T21:03:23Z

The ConfigureUpdateStrategyModal is tightly coupled to deployment workload. For example 'strategy.type field in deployment workload response doesn't match 'updateStrategy.type' in OLM storage cluster. The obj props is named deployment. To start, I created a new model using the based code, trying to figure out how I could use same model for both.
In deployment workload Update Strategy is in the Action dropdown. The bug description wants it in a clickable text. FYI, just thinking if consistency is a concern here
I got an error "No model registered for core.libopenstorage.org~v1alpha1" while working on update strategy save action. Looking into this issue.
Code refactoring in progress

cajieh · 2019-09-30T18:13:29Z

@spadgett
Having issue with the new StorageClusterModel I created for edit update strategy model. I got "No model registered for core.libopenstorage.org~~v1alpha1~~StorageCluster" in console.log and "404: Page Not Found" on the UI. When I changed the kind property in the StorageClusterModel from "StorageCluster" to something else like "StorageCluste" or "StorageCluster2", it works and I am able to edit and save the updateStrategy.

openshift-ci-robot · 2019-09-30T19:03:53Z

@cyril-ui-developer: This pull request references Bugzilla bug 1753812, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

[WIP] Bug 1753812: modify view update strategy descriptor

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

spadgett

@cyril-ui-developer I'd suggest looking at the OLM configure size modal for an example:

https://github.com/openshift/console/blob/master/frontend/packages/operator-lifecycle-manager/src/components/descriptors/spec/configure-size.tsx

frontend/public/models/index.ts

frontend/public/components/modals/configure-update-strategy-modal.tsx

frontend/public/components/modals/index.ts

frontend/public/components/modals/configure-update-strategy-modal.tsx

spadgett · 2019-10-02T18:40:37Z

frontend/packages/operator-lifecycle-manager/src/components/descriptors/spec/index.tsx

-const UpdateStrategy: React.FC<SpecCapabilityProps> = (props) => (
-  <div>{_.get(props.value, 'type', 'None')}</div>
+const UpdateStrategy: React.FC<SpecCapabilityProps> = ({ model, obj, descriptor, value }) => (
+  <button


We're moving to the PF4 <Button variant="link"> as in #2666

cc @sg00dwin

spadgett · 2019-10-02T18:40:59Z

frontend/packages/operator-lifecycle-manager/src/components/descriptors/spec/index.tsx

-  <div>{_.get(props.value, 'type', 'None')}</div>
+const UpdateStrategy: React.FC<SpecCapabilityProps> = ({ model, obj, descriptor, value }) => (
+  <button
+    data-test-id="edit-annotations"


data-test-id looks wrong. We should remove it if it's not used in a test

spadgett · 2019-10-02T18:44:12Z

frontend/packages/operator-lifecycle-manager/src/components/modals/update-strategy-modal.tsx

+    _.get(resource, getPath) || props.defaultValue,
+  );
+  const [maxUnavailable, setMaxUnavailable] = React.useState(
+    _.get(resource.spec, 'updateStrategy.rollingUpdate.maxUnavailable', '25%'),


These paths looks wrong. They should be relative to path. See

https://github.com/spadgett/console/blob/dec402f1c240148efe808ccb1f907478a74e14e6/frontend/packages/operator-lifecycle-manager/src/components/create-operand.tsx#L528-L549

spadgett · 2019-10-02T18:46:47Z

frontend/packages/operator-lifecycle-manager/src/components/modals/update-strategy-modal.tsx

+    )
+      .then(close, () => {})
+      .catch((error) => {
+        throw error;


The catch shouldn't be needed if we're only rethrowing the error

The catch shouldn't be needed if we're only rethrowing the error

Got eslint error when I removed catch - "error Expected catch() or return promise/catch-or-return"
I'm thinking of adding the "promise/catch-or-return": "off" to eslint config file in operator-lifecycle-manager to omit the error or add a return statement to return the promise. Please let me know your thoughts. Thanks.

The catch shouldn't be needed if we're only rethrowing the error

Added the "promise/catch-or-return": "off" to eslint config file in operator-lifecycle-manager to omit the error.

spadgett · 2019-10-02T18:47:24Z

frontend/packages/operator-lifecycle-manager/src/components/modals/update-strategy-modal.tsx

+  const submit = (event) => {
+    event.preventDefault();
+
+    const patch: Patch = { path: '/spec/updateStrategy/rollingUpdate', op: 'remove' };


We shouldn't be hard-coding paths here. It should be based on props.path

spadgett · 2019-10-08T17:19:03Z

frontend/packages/.eslintrc.js

@@ -11,5 +11,6 @@ module.exports = {
        message: 'Use lodash instead. webpack is configured to use lodash-es automatically.',
      },
    ],
+    "promise/catch-or-return": "off",


Hm, are we sure we want this change?

Hm, are we sure we want this change?

Will remove it and use return.

spadgett · 2019-10-08T17:19:51Z

frontend/packages/operator-lifecycle-manager/src/components/modals/update-strategy-modal.tsx

+    }
+
+    handlePromise(
+      k8sPatch(resourceKind, resource, [


I think we do want a return here

spadgett · 2019-10-08T17:23:47Z

frontend/packages/operator-lifecycle-manager/src/components/modals/index.ts

@@ -0,0 +1,6 @@
+// This module utilizes dynamic `import()` to enable lazy-loading for each modal instead of including them in the main bundle.


I'd probably skip this since we don't do it for other modals in the package.

spadgett

/approve
/lgtm

openshift-ci-robot · 2019-10-15T14:22:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cyril-ui-developer, spadgett

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [spadgett]

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

openshift-ci-robot · 2019-10-15T14:49:16Z

@cyril-ui-developer: All pull requests linked via external trackers have merged. Bugzilla bug 1753812 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1753812: modify view update strategy descriptor

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot assigned spadgett Sep 27, 2019

openshift-ci-robot requested review from alecmerdler and benjaminapetersen September 27, 2019 20:43

spadgett changed the title ~~[WIP] modify view update strategy descriptor~~ [WIP] Bug 1753812: modify view update strategy descriptor Sep 30, 2019

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Sep 30, 2019

spadgett requested changes Sep 30, 2019

View reviewed changes

cajieh force-pushed the modify-view-updatestrategy-descriptor branch from dfcec25 to a2bec4f Compare September 30, 2019 22:01

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 30, 2019

cajieh force-pushed the modify-view-updatestrategy-descriptor branch from a2bec4f to 8af04a4 Compare October 1, 2019 19:04

cajieh force-pushed the modify-view-updatestrategy-descriptor branch 3 times, most recently from d6d8811 to 4478cc4 Compare October 2, 2019 18:15

spadgett requested changes Oct 2, 2019

View reviewed changes

cajieh force-pushed the modify-view-updatestrategy-descriptor branch 4 times, most recently from a4512dc to fc26a9d Compare October 7, 2019 14:56

spadgett reviewed Oct 8, 2019

View reviewed changes

cajieh force-pushed the modify-view-updatestrategy-descriptor branch from fc26a9d to 9353cec Compare October 8, 2019 19:07

cajieh changed the title ~~[WIP] Bug 1753812: modify view update strategy descriptor~~ Bug 1753812: modify view update strategy descriptor Oct 8, 2019

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 Oct 8, 2019

modify view update strategy descriptor

88b5f8c

cajieh force-pushed the modify-view-updatestrategy-descriptor branch from 9353cec to 88b5f8c Compare October 15, 2019 13:17

spadgett approved these changes Oct 15, 2019

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 15, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 15, 2019

openshift-merge-robot merged commit 3a421b3 into openshift:master Oct 15, 2019

spadgett added this to the v4.3 milestone Oct 15, 2019

		@@ -0,0 +1,6 @@
		// This module utilizes dynamic `import()` to enable lazy-loading for each modal instead of including them in the main bundle.

Bug 1753812: modify view update strategy descriptor #2864

Bug 1753812: modify view update strategy descriptor #2864

Uh oh!

Conversation

cajieh commented Sep 27, 2019

Uh oh!

cajieh commented Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cajieh commented Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Sep 30, 2019

Uh oh!

spadgett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cajieh Oct 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cajieh Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spadgett left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Oct 15, 2019

Uh oh!

openshift-ci-robot commented Oct 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cajieh commented Sep 27, 2019 •

edited

Loading

cajieh commented Sep 30, 2019 •

edited

Loading

cajieh Oct 7, 2019 •

edited

Loading

cajieh Oct 8, 2019 •

edited

Loading