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

🐙 octavia-cli: use model serialization #12133

Merged
merged 17 commits into from
May 6, 2022

Conversation

alafanechere
Copy link
Contributor

@alafanechere alafanechere commented Apr 19, 2022

What

Some shortcuts taken in the early development of the project challenge the stability of the CLI and lead to issues such as #12270

Problems and solutions

Problem: We discarded the use of model serialization
We disabled model serialization and validation on some API responses because it required updating the Open API config to declare fields as nullable (more about this here). It means the response we handled were raw python dict and not validated models.
Solution: edit airbyte-api/src/main/openapi/config.yaml to make some field nullable. Do not pass _check_return_type=False when calling endpoints to retrieve pure API client models that are validated and expose the convenient model interfaces + python casing. This allows us:

  • Use snake case fields on connection and have consistent casing in generated resources (source / destination / connection). See octavia-cli/integration_tests/configurations/connections/poke_to_pg/configuration.yaml
  • Build valid ConnectionUpdate and ConnectionCreate payloads with the _deserialize_raw_configuration method in octavia-cli/octavia_cli/apply/resources.py. BaseResource instances now have two configuration attributes: raw_configuration which is the content of the YAML configuration serialized as a Python dict and configuration which is the output of _deserialize_raw_configuration. By default it just extract the configuration key of raw_configuration, but for Connection this methods builds a configuration dict with value corresponding to API Client models for validation.

Problem: We used the search endpoint to retrieve remote resources
The search endpoint for connection retrieved inconsistent results when a connection was edited (which led to #12270). We used the search API endpoint in a early stage of the project when we did not persist the resource_id in the state file.
Solution: Now that we have state file we can use the get endpoints to retrieve remote resource from resource_idstored in the state. These change happen in octavia-cli/octavia_cli/apply/resources.py

Other changes

I introduced additional changes to make the code lighter and leverage inheritance as much as possible: I created a SourceAndDestination from BaseResource because Source and Destination share duplicate code otherwise.

🚨 User Impact 🚨

Connection configurations are now using snake_case instead of camel_case. Users with existing connection configuration will have to edit their configuration.yaml file for connections to change the casing.

@github-actions github-actions bot added area/api Related to the api area/platform issues related to the platform labels Apr 19, 2022
@alafanechere alafanechere temporarily deployed to more-secrets April 19, 2022 16:02 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets April 19, 2022 16:02 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets April 29, 2022 13:36 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets April 29, 2022 13:36 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets May 3, 2022 06:50 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets May 3, 2022 06:50 Inactive
@alafanechere alafanechere force-pushed the augustin/octavia/use-model-serialization branch from ab6c416 to 86ccb18 Compare May 3, 2022 06:53
@alafanechere alafanechere temporarily deployed to more-secrets May 3, 2022 06:55 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets May 3, 2022 06:55 Inactive
@alafanechere alafanechere self-assigned this May 3, 2022
@alafanechere alafanechere changed the title Augustin/octavia/use model serialization 🐙 octavia-cli: use model serialization May 3, 2022
@alafanechere alafanechere linked an issue May 3, 2022 that may be closed by this pull request
@alafanechere alafanechere marked this pull request as ready for review May 3, 2022 07:27
@alafanechere alafanechere temporarily deployed to more-secrets May 3, 2022 07:33 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets May 3, 2022 07:33 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets May 4, 2022 15:06 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets May 4, 2022 15:06 Inactive
octavia-cli/README.md Outdated Show resolved Hide resolved
prefix: "" # REQUIRED | Prefix that will be prepended to the name of each stream when it is written to the destination
resourceRequirements: # OPTIONAL | object | Resource requirements to run workers (blank for unbounded allocations)
resource_requirements: # OPTIONAL | object | Resource requirements to run workers (blank for unbounded allocations)
Copy link
Contributor

Choose a reason for hiding this comment

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

Try to catch api exception or validate existing configs to provide a understandable error message to the user directing them to switch to snake_case, or re-generate the files.

Copy link
Contributor

@lmossman lmossman left a comment

Choose a reason for hiding this comment

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

Overall looks good me, if you want me to take another look after making the validation of snake_case changes, let me know

@alafanechere alafanechere temporarily deployed to more-secrets May 5, 2022 12:45 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets May 5, 2022 12:45 Inactive
@CLAassistant
Copy link

CLAassistant commented May 5, 2022

CLA assistant check
All committers have signed the CLA.

@alafanechere alafanechere temporarily deployed to more-secrets May 5, 2022 13:23 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets May 5, 2022 13:23 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets May 5, 2022 13:26 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets May 5, 2022 13:26 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets May 5, 2022 16:54 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets May 5, 2022 16:54 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets May 6, 2022 07:12 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets May 6, 2022 07:12 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets May 6, 2022 10:23 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets May 6, 2022 10:23 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets May 6, 2022 16:20 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets May 6, 2022 16:20 Inactive
@alafanechere alafanechere merged commit 07068b0 into master May 6, 2022
@alafanechere alafanechere deleted the augustin/octavia/use-model-serialization branch May 6, 2022 16:55
@alafanechere alafanechere temporarily deployed to more-secrets May 6, 2022 16:57 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets May 6, 2022 16:57 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Related to the api area/platform issues related to the platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 octavia-cli: connections re-created on apply
3 participants