-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
🐙 octavia-cli: use model serialization #12133
Conversation
ab6c416
to
86ccb18
Compare
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this 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
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:ConnectionUpdate
andConnectionCreate
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 andconfiguration
which is the output of_deserialize_raw_configuration
. By default it just extract theconfiguration
key ofraw_configuration
, but forConnection
this methods builds aconfiguration
dict with value corresponding to API Client models for validation.Problem: We used the
search
endpoint to retrieve remote resourcesThe
search
endpoint for connection retrieved inconsistent results when a connection was edited (which led to #12270). We used thesearch
API endpoint in a early stage of the project when we did not persist theresource_id
in thestate
file.Solution: Now that we have
state
file we can use theget
endpoints to retrieve remote resource fromresource_id
stored in thestate
. These change happen in octavia-cli/octavia_cli/apply/resources.pyOther changes
I introduced additional changes to make the code lighter and leverage inheritance as much as possible: I created a
SourceAndDestination
fromBaseResource
becauseSource
andDestination
share duplicate code otherwise.🚨 User Impact 🚨
Connection configurations are now using
snake_case
instead ofcamel_case
. Users with existing connection configuration will have to edit theirconfiguration.yaml
file for connections to change the casing.