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

🎉 🪟 [Connector Builder] Resolve manifest before converting to builder form values #21898

Merged
merged 4 commits into from
Jan 27, 2023

Conversation

lmossman
Copy link
Contributor

What

Closes #21610

Now that we have an endpoint in the connector builder server to resolve all $refs and $options in a manifest, this PR makes the connector builder call that endpoint on the YAML manifest in the editor before converting it to UI.

How

  • Add useResolveManifest to the ConnectorBuilderApiService to make the new endpoint usable
  • Change manifestToBuilderForm.ts to a custom hook useManifestToBuilderForm.ts, since it needs to use the useResolveManifest() hook now
  • Modify convertToBuilderFormValues to call the resolve endpoint at the beginning of the function, and use the result in the rest of the conversion
  • The above change resulted in that method becoming async, so update its usages and the tests to be async as well

@octavia-squidington-iv octavia-squidington-iv added the area/frontend Related to the Airbyte webapp label Jan 26, 2023
@@ -234,7 +164,7 @@ describe("Conversion successfully results in", () => {
]);
});

it("spec properties converted to input overrides on matching auth keys", () => {
it("spec properties converted to input overrides on matching auth keys", async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A lot of small async/await changes in this file, since convertToBuilderFormValues is now an async function

@flash1293
Copy link
Contributor

flash1293 commented Jan 26, 2023

This is super nice! Trying this with real manifest I found a few generic issues in our conversion code: https://docs.google.com/document/d/1694N09qm839yEtsSrGfC1ptDCickVm9cQWsSYBSiT-Q/edit?usp=sharing

I don't think we need to solve all of them and most likely on a separate PR, but in order to make the "upload a yaml file" CTA useful I think we should solve the most critical ones.

If you agree I can split it into issues

@lmossman
Copy link
Contributor Author

@flash1293 thanks for compiling this! I agree we should solve these, though I think we can merge this PR in in its current state and split the doc into tickets as you suggested 👍

@flash1293
Copy link
Contributor

I think something went wrong with the auto-formatting

@lmossman
Copy link
Contributor Author

Woah, yep you're right. I'll work on fixing

@lmossman lmossman force-pushed the lmossman/resolve-refs-before-convert branch from f9c1b89 to 6a940d6 Compare January 26, 2023 19:01
@lmossman
Copy link
Contributor Author

@flash1293 this should be fixed now

@lmossman lmossman requested review from flash1293 and removed request for flash1293 January 26, 2023 19:39
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

LGTM

@lmossman lmossman merged commit 9583fc1 into master Jan 27, 2023
@lmossman lmossman deleted the lmossman/resolve-refs-before-convert branch January 27, 2023 17:32
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Connector Builder] Use ref resolving endpoint before converting YAML to UI
3 participants