Skip to content

Conversation

@divyanshiGupta
Copy link
Contributor

@divyanshiGupta divyanshiGupta commented Jan 28, 2020

Jira Issue - https://issues.redhat.com/browse/ODC-2853

After formik upgrade to 2.0.1-rc.5 this error jaredpalmer/formik#1678 started to pop on forms where it is needed to populate the form fields with existing values and an existing event handler is used inside useEffect to achieve this.

For example dev-console edit flows (In deploy image internal registry edit flow imageStreamNamespace is already set and onDropdownChange is called to fill the other fields and dispatch desired actions according to the namespace).

formik upgrade is needed for dev-console edit flows:

Solution:
Upgrade to 2.1.2
It gives this error -
image
To resolve this we need to upgrade @types/react and it gives another lot of errors on upgrade.

Upgrade to 2.0.3
It solves the formik issue jaredpalmer/formik#1678 and does not give the above mentioned type error and hence no need of upgrading @types/react.

@rohitkrai03 Which approach seems better? I think upgrading to 2.0.3 will be better for the time being

cc: @christianvogt

@openshift-ci-robot 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/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 28, 2020
@openshift-ci-robot openshift-ci-robot added component/core Related to console core functionality component/dev-console Related to dev-console labels Jan 28, 2020
@divyanshiGupta
Copy link
Contributor Author

/retest

1 similar comment
@divyanshiGupta
Copy link
Contributor Author

/retest

@spadgett
Copy link
Member

ERROR in /go/src/github.com/openshift/console/frontend/node_modules/formik/dist/Form.d.ts
(3,150): Type '"hidden" | "dir" | "slot" | "style" | "title" | "color" | "key" | "children" | "prefix" | "acceptCharset" | "action" | "autoComplete" | "encType" | "method" | "name" | "noValidate" | ... 246 more ... | "translate"' does not satisfy the constraint '"hidden" | "dir" | "slot" | "style" | "title" | "color" | "key" | "ref" | "children" | "prefix" | "acceptCharset" | "action" | "autoComplete" | "encType" | "method" | "name" | "noValidate" | ... 246 more ... | "onTransitionEndCapture"'.
  Type '"translate"' is not assignable to type '"hidden" | "dir" | "slot" | "style" | "title" | "color" | "key" | "ref" | "children" | "prefix" | "acceptCharset" | "action" | "autoComplete" | "encType" | "method" | "name" | "noValidate" | ... 246 more ... | "onTransitionEndCapture"'.

@divyanshiGupta
Copy link
Contributor Author

divyanshiGupta commented Jan 28, 2020

Yes already mentioned the error in the description. Shouldnt have retested because obviously the tests will fail.

@rohitkrai03
Copy link
Contributor

@divyanshiGupta I think we can go ahead with upgrade to 2.0.3 for now and later upgrade to latest with all the required changes.

@divyanshiGupta divyanshiGupta changed the title [WIP]Bump formik Bump formik Jan 29, 2020
@openshift-ci-robot 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 29, 2020
Copy link
Contributor

@rohitkrai03 rohitkrai03 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2020
@andrewballantyne
Copy link
Contributor

/kind bug

@divyanshiGupta looks like you have a test break around Formik components.

@openshift-ci-robot openshift-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed lgtm Indicates that a PR is ready to be merged. labels Jan 29, 2020
@divyanshiGupta
Copy link
Contributor Author

@andrewballantyne fixed the test.

@andrewballantyne
Copy link
Contributor

@divyanshiGupta Awesome, you'll need to get Christian or someone from Admin to approve it as it reached outside of our teams packages for me to approve.

@divyanshiGupta
Copy link
Contributor Author

/assign @christianvogt

@divyanshiGupta
Copy link
Contributor Author

@andrewballantyne you can add lgtm as it got removed because of the test fix.

@rohitkrai03
Copy link
Contributor

/lgtm

Comment on lines +8 to +12
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@divyanshiGupta divyanshiGupta Jan 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@christianvogt after upgrade it was throwing error on line 18,27,37 and 46 that the defaultRequestUnit does not exist on type string, I tried to do a bunch of things but the error wont go, then I tried to use the same values from useFormikContext and the error was gone.

Then I went through the implementation and they have changed the implementation of getFieldProps (used inside useField to get the field values) a bit from 2.0.1rc5 to 2.0.3. Now in 2.0.3 its enforced that we need to pass an object to useField for field types such as checkbox to get the desired data, but for other field types such TextInput passing a string of field name is sufficient. For full context you will have to go through the above 2 links I posted.

I missed to update the ToggleableFieldBase though, will do it. But I think for our use case here its okay to use useFormikContext in ResourceLimitSection since its not a FormikField and useField, if you see the implementation makes more sense when used in a FormikField.

@divyanshiGupta divyanshiGupta changed the title Bump formik [WIP]Bump formik Jan 31, 2020
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 31, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 31, 2020
@divyanshiGupta divyanshiGupta changed the title [WIP]Bump formik Bump formik Jan 31, 2020
@openshift-ci-robot openshift-ci-robot added component/shared Related to console-shared and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jan 31, 2020
@divyanshiGupta
Copy link
Contributor Author

Updated the ToggleableFieldBase according to the changes made in useField in 2.0.3.

@christianvogt
Copy link
Contributor

fyi @jtomasek

@christianvogt
Copy link
Contributor

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 3, 2020
@christianvogt
Copy link
Contributor

@divyanshiGupta please add a bz

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, divyanshiGupta, rohitkrai03

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:

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 openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 3, 2020
@openshift-bot
Copy link
Contributor

/retest

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

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

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

@divyanshiGupta divyanshiGupta changed the title Bump formik Bug 1798357: Bump formik Feb 5, 2020
@openshift-ci-robot openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Feb 5, 2020
@openshift-ci-robot
Copy link
Contributor

@divyanshiGupta: This pull request references Bugzilla bug 1798357, which is invalid:

  • expected the bug to target the "4.4.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

Bug 1798357: Bump formik

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.

@divyanshiGupta
Copy link
Contributor Author

/bugzilla refresh

@openshift-ci-robot openshift-ci-robot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Feb 5, 2020
@openshift-ci-robot
Copy link
Contributor

@divyanshiGupta: This pull request references Bugzilla bug 1798357, 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:

/bugzilla refresh

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-merge-robot openshift-merge-robot merged commit 06db10e into openshift:master Feb 5, 2020
@openshift-ci-robot
Copy link
Contributor

@divyanshiGupta: All pull requests linked via external trackers have merged. Bugzilla bug 1798357 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1798357: Bump formik

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 spadgett added this to the v4.4 milestone Feb 6, 2020
@spadgett spadgett added the kind/dependency-change Categorizes issue or PR as related to changing dependencies label Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. component/core Related to console core functionality component/dev-console Related to dev-console component/shared Related to console-shared kind/bug Categorizes issue or PR as related to a bug. kind/dependency-change Categorizes issue or PR as related to changing dependencies lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants