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] Set placeholder documentation_url for builder-generated spec #22340

Merged
merged 2 commits into from
Feb 6, 2023

Conversation

lmossman
Copy link
Contributor

@lmossman lmossman commented Feb 2, 2023

What

See slack context: https://airbytehq-team.slack.com/archives/C03TP6W8081/p1675365814016559?thread_ts=1675198798.481999&cid=C03TP6W8081

Manifests generated by the connector builder currently do not pass integration tests and cannot be used, because they have an empty documentation_url.

This issue platform issue will handle fixing the airbyte-server to not fail in this case: #22242
But until that is complete, we should have an intermediate solution here to fix the problem in the meantime.

How

This PR addresses the issue by setting documentation_url in the manifest produce by the connector builder to the placeholder documentation url we have used in other connectors so far.

This can be removed once #22242 is complete

@octavia-squidington-iii octavia-squidington-iii added the area/frontend Related to the Airbyte webapp label Feb 2, 2023
@lmossman lmossman requested review from flash1293 and girarda February 2, 2023 19:24
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.

Makes sense, thanks for fixing. I'm wondering whether we should have an integration test for this path (configure something, download yaml, then make it work), but maybe it's not worth it as the end to end flow in the UI will cover it will enough once that's implemented

@@ -636,7 +636,7 @@ export const convertToManifest = (values: BuilderFormValues): ConnectorManifest

const spec: Spec = {
connection_specification: specSchema,
documentation_url: "",
documentation_url: "https://docsurl.com",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: What about using https://example.org/ for this? Its official purpose is to be used as an example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that suggestion, will update

@lmossman
Copy link
Contributor Author

lmossman commented Feb 6, 2023

Yeah I think an E2E test to do that whole flow in the UI makes sense to add once we have the full flow implemented

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.

3 participants