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

Formik upgrade #3469

Closed
wants to merge 2 commits into from
Closed

Formik upgrade #3469

wants to merge 2 commits into from

Conversation

jtomasek
Copy link

@jtomasek jtomasek commented Nov 19, 2019

Upgrade Formik to version 2.0.3

Required by #3516

@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 Nov 19, 2019
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. component/dev-console Related to dev-console component/metal3 Related to metal3-plugin labels Nov 19, 2019
@jtomasek
Copy link
Author

/retest

@openshift-ci-robot openshift-ci-robot added the component/core Related to console core functionality label Nov 21, 2019
@jtomasek jtomasek changed the title [WIP] Formik upgrade Formik upgrade Nov 21, 2019
@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 Nov 21, 2019
@christianvogt
Copy link
Contributor

cc @rohitkrai03

Copy link
Contributor

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

Upgrade looks good. Please to see the refactor PR make it in that depends on this PR.

/lgtm

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

@jtomasek Any specific reason for upgrading Formik? I had kept the formik version to rc1 because of an issue with low priority validations being done on stale values which would result in error message of that input field to appear late.

Few related issues on formik repository -

Weirdly enough the issue has not been fixed even after 13 rcs and a few more releases of formik after that. Just tested the PR for it and you can see the effects of this bug in below screenshot. When the scaling input goes to -1 it should show error but it doesn't and shows error the next time when value is canged to -2.

ezgif com-video-to-gif (4)

We would need to spend some time finding a solution that works for us for this specific issue after the upgrade.

cc: @christianvogt @andrewballantyne

@andrewballantyne
Copy link
Contributor

It looks like there is some workarounds in those issues (such as invoking the validation ourselves - formik.validateForm). We could look to implement one of those while the Formik works on an official solution? @rohitkrai03

@jtomasek
Copy link
Author

jtomasek commented Dec 3, 2019

My goal was simply to move from RCs and upgrade Formik to the new 'stable' 2.0.3 version and add new multi checkbox functionality. All this before moving the components tho shared package.

@andrewballantyne
Copy link
Contributor

My goal was simply to move from RCs and upgrade Formik to the new 'stable' 2.0.3 version and add new multi checkbox functionality. All this before moving the components tho shared package.

@jtomasek Not that I am currently asking you to do so... but I am wondering, could you do the migration to shared without upgrading (you'd have to undo the checkbox/switch changes is the only thing that comes to mind). Or is making that change now going to just slam you with conflicts?

@jtomasek
Copy link
Author

jtomasek commented Dec 3, 2019

My goal was simply to move from RCs and upgrade Formik to the new 'stable' 2.0.3 version and add new multi checkbox functionality. All this before moving the components tho shared package.

@jtomasek Not that I am currently asking you to do so... but I am wondering, could you do the migration to shared without upgrading (you'd have to undo the checkbox/switch changes is the only thing that comes to mind). Or is making that change now going to just slam you with conflicts?

No major conflicts it seems. I can rebase that PR so it does not depend on this upgrade.

@andrewballantyne
Copy link
Contributor

No major conflicts it seems. I can rebase that PR so it does not depend on this upgrade.

I leave it up to you - I think the move to shared can be done independent of this upgrade. Might streamline the change if you broke it up.

@jtomasek
Copy link
Author

jtomasek commented Dec 3, 2019

Done (#3516).

@rohitkrai03
Copy link
Contributor

It looks like there is some workarounds in those issues (such as invoking the validation ourselves - formik.validateForm). We could look to implement one of those while the Formik works on an official solution? @rohitkrai03

@andrewballantyne I did try using formik.validateForm back when I encountered the issue first but it seems the validateForm function is passed the stale formik values as well. That is why I reverted back to using rc-1 release. I haven't tested this again with latest release yet, will have to investigate a bit further to know if that was fixed at least.

@andrewballantyne
Copy link
Contributor

Since this is separated now, perhaps we address this issue before upgrading.

/lgtm cancel

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 3, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jtomasek
To complete the pull request process, please assign bparees
You can assign the PR to them by writing /assign @bparees in a comment when ready.

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

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

@christianvogt
Copy link
Contributor

@jtomasek is this still needed? Is it worth salvaging anything here in the current PR or closing and creating a new PR?

@openshift-bot
Copy link
Contributor

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci-robot openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 4, 2020
@openshift-ci-robot
Copy link
Contributor

@jtomasek: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/kubevirt-plugin e386694 link /test kubevirt-plugin

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@andrewballantyne
Copy link
Contributor

/close

Looks like this is not going to go in, closing PR.

@openshift-ci-robot
Copy link
Contributor

@andrewballantyne: Closed this PR.

In response to this:

/close

Looks like this is not going to go in, closing PR.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/core Related to console core functionality component/dev-console Related to dev-console component/metal3 Related to metal3-plugin lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. 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.

None yet

6 participants