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

🪟 🐛 Hotfix getValues failures in connector form #19400

Merged
merged 5 commits into from
Nov 15, 2022

Conversation

timroes
Copy link
Collaborator

@timroes timroes commented Nov 14, 2022

What

Hotfix for https://github.com/airbytehq/oncall/issues/1050

Makes sure we're not failing the ConnectorForm in case the getValues cast fails due to some issues (we seem to have some issues where the values are not cleared out properly and such clash with the validation schema while casting).

@timroes timroes requested a review from a team as a code owner November 14, 2022 22:54
@octavia-squidington-iv octavia-squidington-iv added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Nov 14, 2022
Comment on lines +149 to +152
// We still see cases where calling `getValues` which will eventually use the yupSchema.cast method
// crashes based on the passed in values. To temporarily make sure we're not crashing the form, we're
// falling back to `values` in case the cast fails. This is a temporary patch, and we need to investigate
// all the failures happening here properly.
Copy link
Contributor

Choose a reason for hiding this comment

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

To temporarily make sure we're not crashing the form, we're falling back to values in case the cast fails.

What are the consequences of falling back to values in this case? Does this just mean that default values will not be inserted, or are there any others?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

afaik it should only cause the default values to not be present, and just have the Formik values "as it". Ideally we still figure out quick enough what cases might fail here (since the weird S3 spec might not the only one) and try to fix them properly, but that way we have at least a chance it's still working "largely" (given that even without the default value, we saw most OAuth connectors working).

Copy link
Contributor

@lmossman lmossman left a comment

Choose a reason for hiding this comment

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

Changes LGTM, as we discussed over zoom. I tested locally the S3 destination and the behavior looked like it was working as it did over zoom.

I tried out other several other connectors as well, and they all seemed to work as expected from what I could tell. So I think this is fine to merge and deploy in the morning!

@timroes timroes merged commit 6617229 into master Nov 15, 2022
@timroes timroes deleted the tim/hotfix-connector-form branch November 15, 2022 10:33
akashkulk pushed a commit that referenced this pull request Dec 2, 2022
* Hotfix getValues failures in connector form

* Fix jest tests

* Fix jest tests

* Show errors somewhat properly

* Fix jest tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to the Airbyte webapp area/platform issues related to the platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants