🐙 octavia-cli: generate open api client #9277

alafanechere · 2022-01-04T11:09:06Z

What

We need an API client to make octavia-cli talk with Airbyte's API.
Airbyte API's uses Open API specs, making it easy to autogenerate a python client with openapitools/openapi-generator-cli.

How

Create a script to generate the API client using the openapi-generator-cli docker image from airbyte-api/src/main/openapi/config.yaml.
Call this script during the build in octavia-cli/build.gradle.
Make small changes to airbyte-api/src/main/openapi/config.yaml
Run the client generation -> creates a new python module in octavia-cli/airbyte_api_client
Use the client in octavia command group to retrieve the Airbyte workspace's id
Unit test the behavior of step 5.

Now running octavia --airbyte-url https://demo.airbyte.io list outputs:

🐙 - Octavia is targetting your Airbyte instance running at https://demo.airbyte.io on workspace b734c3d7-ece6-47e0-8f07-c4be707fbcfa
Error: The list command is not yet implemented.

🚨 User Impact 🚨

No impact as the

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

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
After the connector is published, connector added to connector index as described here
Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as 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

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
After the new connector version is published, connector version bumped in the seed directory as described here
Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as 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.

This change is

octavia-cli/octavia_cli/entrypoint.py

cgardens

only blocking because i don't understand the chnages in airbyte-api/src/main/openapi/config.yaml. otherwise it looks good! nice use of the docker open api generator container!

cgardens · 2022-01-10T15:38:09Z

airbyte-api/src/main/openapi/config.yaml

@@ -1944,8 +1944,10 @@ components:
            $ref: "#/components/schemas/Notification"
        firstCompletedSync:
          type: boolean
+          nullable: true


what are you trying to achieve here? because these fields are not in the required list above they should already be nullable and should not require the extra nullable field.

When I called the workspace endpoint with the generated API client it raises an error during serialization of the response because it expected this field to be not nullable. When I make this change in config.yaml the problem is bypassed. I'm going to check if I can find another solution by tweaking some generator options, otherwise, I'll have to make these kinds of changes in config.yaml for other endpoints that will be used in the future, which is not ideal.

yeah. that seems like kinda big deal. swagger generation is always a little jenky around the edges, but nullable fields is a pretty core feature, so it seems like a really bad sign if the generator can't handle it because it means there will be other problems.

one thing to test is instead of using the nullable field if instead setting type: [boolean, null] works. I don't like this as much as the pattern we already use but if we needed to switch to this pattern it may not be the end of the world.

I found this issue OpenAPITools/openapi-generator#4816 on OpenApiTools repo, this is for their C# generator but it looks like the exact same problem I have with the Python one. I'll continue to investigate and try type: [boolean, null]. Let me know if you think that I should rather implement the client myself.

we definitely don't want to implement it ourselves. my guess is the C# bug is language specific (since C# is actually a typed language)

I tried [boolean, null] but this is not a valid spec according to open-api-generator.
I think I found a workaround. I can disable the type validation of the response. It also disables deserialization to a model and leads us to manipulate the raw response as a python dict. It's not the most elegant solution but I prefer this rather than editing the API spec. Related commit: a586719
I also reverted my changes on airbyte-api's config.yaml.

.pre-commit-config.yaml

octavia-cli/README.md

cgardens · 2022-01-10T15:44:07Z

octavia-cli/build.gradle

@@ -7,3 +7,12 @@ airbytePython {
    moduleDirectory 'octavia_cli'
 }

+task generateApiClient(type: Exec) {


does the caching work properly on this task? the expectation should be that if airbyte-api/src/main/openapi/config.yaml does not change that this task does not need to run again. if that's not set up properly it will require re-executing this task and every tasks that depends on them (which is most of them).

see UP-TO-DATE in https://docs.gradle.org/current/userguide/more_about_tasks.html

Thanks for this suggestion! I realized I could use a grade plugin already used for java API client generation: org.openapitools.generator.gradle.plugin.tasks.GenerateTask. It supports incremental builds, but I can't figure why it's not working in my context.
I did the following in b285449:

update octavia-cli/build.gradle to use the GenerateTask.

exclude the generated client from the license headers checks.

remove the script I wrote to generate the client with the docker image.

Still, I don't get why I can't see the UP-TO-DATE flag after consequent builds.

I finally made the FROM-CACHE and UP-TO-DATE mechanisms work. The solution was to use the buildDir as output dir. Using other paths made Gradle re-run the task on each run. The generated client is now stored in octavia-cli/build/airbyte_api_client. Related commit: e95abc9

I confirm that changes in airbyte-api/src/main/openapi/config.yaml will trigger a regeneration of the client on SUB_BUILD=OCTAVIA_CLI ./gradlew build.

I'd like you to present this very briefly in the dev meeting. This is something we often get wrong when using gradle, so it would be good to have a 5 minute presentation to share the knowledge with the team.

ok! To be honest, I'm not understanding why using input paths for tasks that are not buildDir does not lead to UP-TO-DATE but it could indeed be interesting to share this finding and to open this discussion in the dev meeting.

cgardens · 2022-01-10T15:46:36Z

octavia-cli/unit_tests/test_entrypoint.py

-def test_octavia():
+@click.command()
+@click.pass_context
+def dumb(ctx):


cgardens

looks good! don't love that we can't used the typed objects in python, but i agree with you, i can't think of another workaround.

alafanechere temporarily deployed to more-secrets January 4, 2022 11:09 Inactive

github-actions bot added area/api Related to the api area/platform issues related to the platform labels Jan 4, 2022

alafanechere removed the area/api Related to the api label Jan 4, 2022

Base automatically changed from augustin/octavia-cli/bootstrap-repo to master January 5, 2022 20:19

alafanechere changed the base branch from master to 0.29.12-alpha January 6, 2022 08:14

alafanechere changed the base branch from 0.29.12-alpha to master January 6, 2022 08:14

alafanechere added 6 commits January 6, 2022 09:18

add script to generate api client

e217327

declare nullable values in openapi config

e45096e

generate api client with build

f2ed2f9

add airbyte_api_client as dep

d9eb674

call workspace endpoint from octavia command group

c15835f

test octavia command group

2afe6c2

alafanechere force-pushed the augustin/octavia-cli/open-api-client branch from d0c8f67 to 2afe6c2 Compare January 6, 2022 08:19

github-actions bot added the area/api Related to the api label Jan 6, 2022

alafanechere temporarily deployed to more-secrets January 6, 2022 08:21 Inactive

update readme

edf3132

alafanechere temporarily deployed to more-secrets January 6, 2022 08:26 Inactive

do not version the api client

690c75a

alafanechere temporarily deployed to more-secrets January 6, 2022 09:12 Inactive

format

0d5ad55

alafanechere force-pushed the augustin/octavia-cli/open-api-client branch from 9cb23df to 0d5ad55 Compare January 6, 2022 09:15

format

3758b53

alafanechere temporarily deployed to more-secrets January 6, 2022 09:17 Inactive

do not delete ci_static_check_reports

5732526

alafanechere temporarily deployed to more-secrets January 6, 2022 09:20 Inactive

fix not implemented error messages

c3f0d89

alafanechere temporarily deployed to more-secrets January 6, 2022 09:25 Inactive

update README

f746be2

alafanechere temporarily deployed to more-secrets January 6, 2022 09:28 Inactive

alafanechere marked this pull request as ready for review January 6, 2022 09:33

alafanechere requested review from marcosmarxm, cgardens and harshithmullapudi January 6, 2022 09:34

alafanechere added area/octavia-cli and removed area/platform issues related to the platform labels Jan 6, 2022

thomasclavet reviewed Jan 10, 2022

View reviewed changes

octavia-cli/octavia_cli/entrypoint.py Outdated Show resolved Hide resolved

cgardens requested changes Jan 10, 2022

View reviewed changes

use gradle openapitools GenerateTask to benefit from incremental builds

b285449

github-actions bot added the area/platform issues related to the platform label Jan 11, 2022

delete generate-api-client.sh

6d1444b

alafanechere temporarily deployed to more-secrets January 11, 2022 10:29 Inactive

make incremental builds work

e95abc9

alafanechere temporarily deployed to more-secrets January 12, 2022 10:10 Inactive

update readme

643c1aa

alafanechere temporarily deployed to more-secrets January 12, 2022 10:18 Inactive

alafanechere added 3 commits January 13, 2022 09:45

revert changes on airbyte-api/src/main/openapi/config.yaml

d2e5382

bypass response validation for list_workspace

a586719

update unit tests and use mocker

b306dec

github-actions bot removed area/platform issues related to the platform area/api Related to the api labels Jan 13, 2022

alafanechere temporarily deployed to more-secrets January 13, 2022 08:51 Inactive

alafanechere requested a review from cgardens January 13, 2022 08:58

cgardens approved these changes Jan 17, 2022

View reviewed changes

alafanechere merged commit d65b8f9 into master Jan 17, 2022

alafanechere deleted the augustin/octavia-cli/open-api-client branch January 17, 2022 08:27

octavia-squidington-iii mentioned this pull request Jan 20, 2022

Bump Airbyte version from 0.35.5-alpha to 0.35.6-alpha #9630

Merged

alafanechere added the user-support-eng label Jan 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐙 octavia-cli: generate open api client #9277

🐙 octavia-cli: generate open api client #9277

alafanechere commented Jan 4, 2022 •

edited by lmossman

Loading

cgardens left a comment

cgardens Jan 10, 2022

alafanechere Jan 11, 2022 •

edited

Loading

cgardens Jan 12, 2022

alafanechere Jan 12, 2022

cgardens Jan 12, 2022

alafanechere Jan 13, 2022

cgardens Jan 10, 2022

alafanechere Jan 11, 2022 •

edited

Loading

alafanechere Jan 12, 2022 •

edited

Loading

cgardens Jan 12, 2022

cgardens Jan 12, 2022

alafanechere Jan 13, 2022

cgardens Jan 10, 2022

cgardens left a comment

🐙 octavia-cli: generate open api client #9277

🐙 octavia-cli: generate open api client #9277

Conversation

alafanechere commented Jan 4, 2022 • edited by lmossman Loading

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

cgardens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alafanechere Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alafanechere Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

alafanechere Jan 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgardens left a comment

Choose a reason for hiding this comment

alafanechere commented Jan 4, 2022 •

edited by lmossman

Loading

alafanechere Jan 11, 2022 •

edited

Loading

alafanechere Jan 11, 2022 •

edited

Loading

alafanechere Jan 12, 2022 •

edited

Loading