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

Add the /manifest_template endpoint to return a string representation of the manifest template #19367

Merged
merged 3 commits into from
Nov 14, 2022

Conversation

brianjlai
Copy link
Contributor

What

The UI needs to display the default manifest template the first time a developer enters the connector builder UI. This is just a flat string which will then be rendered by the UI. It includes indentation spaces and \n characters.

How

Just returns a flat string. It is a little bit annoying that we have the template mostly duplicated in the generator and in the connector builder server, but there are a few slight changes between the files. One example is we need to escape the interpolation block in the generator version. But in the connector server one, we don't need to.

Recommended reading order

  1. default_api.py

http_method: "GET"
authenticator:
type: BearerAuthenticator
api_token: "{{ config['api_key'] }}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned. In the generator this is defined as /{{ config['api_key'] }} because we need to escape the template so that the template generator doesn't fill this with anything. We want the {{ }} to actually be present in the file. But for the server version, we don't need to escape anything

Copy link
Contributor

Choose a reason for hiding this comment

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

One way that you could reduce the duplication here would be to have a shared python method which takes in arguments for the api_token value and the spec title value, and returns this string with those arguments inserted. Then this method could call it and pass in the api_token without the slash, and the generator could call it and pass in the api_token with the slash, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm yeah that sounds like it could work, but I might table that type of change for the future, I don't think its necessarily critical to keep things perfectly in sync in V0. And we could have some thorny edges trying to read the YAML template from airbyte-integrations/connector-templates/source-configuration-based/source_{{snakeCase name}}/{{snakeCase name}}.yaml.hbs across python packages 😅

spec:
documentation_url: https://docsurl.com
connection_specification:
title: Source Name Spec # 'TODO: Replace this with the name of your source.'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this used to be {{capitalCase name}} in the generator. But it could be misleading for a developer thinking it should be interpolated when its not supposed to be.

@brianjlai brianjlai temporarily deployed to more-secrets November 13, 2022 02:39 Inactive
@brianjlai brianjlai marked this pull request as ready for review November 14, 2022 17:02
@brianjlai brianjlai merged commit b52caa0 into master Nov 14, 2022
@brianjlai brianjlai deleted the brian/connector_builder_manifest_template branch November 14, 2022 23:40
akashkulk pushed a commit that referenced this pull request Dec 2, 2022
… of the manifest template (#19367)

* return a string representation of the manifest template

* get rid of extra template

* add a comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants