oAuth flow: Add input fields parameter.

[avida]

What

Part of #6971

How

Describe the solution

Recommended reading order

1. x.java
2. y.python

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

• Community member? Grant edit access to maintainers (instructions)
• Secrets in the connector's spec are annotated with airbyte_secret
• Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
• Code reviews completed
• Documentation updated
  • Connector's README.md
  • Connector's bootstrap.md. See description and examples
  • docs/SUMMARY.md
  • docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
  • docs/integrations/README.md
  • airbyte-integrations/builds.md
• PR name follows PR naming conventions
• Connector added to connector index like described here

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

• Create a non-forked branch based on this PR and test the below items on it
• Build is successful
• Credentials added to Github CI. Instructions.
• /test connector=connectors/<name> command is passing.
• New Connector version released on Dockerhub by running the /publish command described here

Updating a connector

Community member or Airbyter

• Grant edit access to maintainers (instructions)
• Secrets in the connector's spec are annotated with airbyte_secret
• Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
• Code reviews completed
• Documentation updated
  • Connector's README.md
  • Connector's bootstrap.md. See description and examples
  • Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
• PR name follows PR naming conventions
• Connector version bumped like described here

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

• Create a non-forked branch based on this PR and test the below items on it
• Build is successful
• Credentials added to Github CI. Instructions.
• /test connector=connectors/<name> command is passing.
• New Connector version released on Dockerhub by running the /publish command described here

Connector Generator

• Issue acceptance criteria met
• PR name follows PR naming conventions
• If adding a new generator, add it to the list of scaffold modules being tested
• The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
• Documentation which references the generator is updated as needed.

[avida]

This is backend part for oAuth input parameters, UI changes will come in separate PR later

[cgardens]

cool!

[cgardens]

can we snake case this please

[cgardens]

also this pattern seems a little bit weird to me. what we really want is to just describe another object that happens to be a subset of the connector spec. would it make any sense to just define a separate json schema instead of doing the pointers. the downside is it adds complexity in mapping from one object to the next.

@sherifnada dunno if you had strong opinions on this.

[sherifnada]

It's definitely a lot simpler to authorize that way since it also allows us to decouple oauth flow params from connector params (if in the future a user needs to input something required only for oauth and not for the connector to operate then we have no way to do it currently). But I can see that approach being complicated for the UI to implement especially if a connector supports more than just oauth.

@jamakase do you have any thoughts on it?

[avida]

can we snake case this please

Ive made it to look similar to other fields in section: oauthFlowInitParameters, oauthFlowOutputParameters. Not sure if I should rename those fields cause it would require changing a lot of code and connectors specs.

[jamakase]

@sherifnada I have already provided all my concerns in slack that I am not a fan of current implementation :) What if in the future some fields should be part of oauth flow, but they are not required to be part of connectionConfigration? The current config won't allow such changes and we would need to add patches within the current schema for such approach what is not that good I think.

[sherifnada]

@jrhizor it seems like you an @jamakase had a different preference for how this should be structured? Right now it's a list of pointers into fields in the config, but I believe you mentioned in slack a preference for having a separate "sub-spec"? IMO from a UX standpoint it's ideal if the user only had to enter their info once, but I can also see that having multiple specs is more extensible as it allows specifying params exclusive to oauth and not the connector as a whole

[avida]

but I can also see that having multiple specs is more extensible as it allows specifying params exclusive to oauth and not the connector as a whole

If we dont like list of pointers approach (which is completely fine from my point of view) we could use custom metadata keywords for existing spec instead of using one more jsonschema .

[jamakase]

@jrhizor it seems like you an @jamakase had a different preference for how this should be structured? Right now it's a list of pointers into fields in the config, but I believe you mentioned in slack a preference for having a separate "sub-spec"? IMO from a UX standpoint it's ideal if the user only had to enter their info once, but I can also see that having multiple specs is more extensible as it allows specifying params exclusive to oauth and not the connector as a whole

My opinion is that our current UI doesn't match our needs actually. We don't want to provide duplicate fields for sure, but the problem is that we may also have additional fields + I think it could make sense to split form into something that is auth specific and something that is connection itself specific ( date of start, some other additional props ). Just like IntellIj does for db connection.

I think, we should try to plan schema correctly, so it support all our future needs and if necessary I could help @avida with any required changes into his PR that will be required to no display duplicate fields. If we have 2 specs - I do not thnk it will be a big deal to merge them properly.

[avida]

@cgardens @jamakase @sherifnada @jrhizor Guys, what is the status of this discussion? Do you have any updates? This issue blocks a lot of oAuth implementations.

[sherifnada]

@avida @ChristopheDuong is working on resolving this asap, let's focus on connectors not blocked by this in the meantime

[avida]

Chris is working on it