🐙 octavia-cli: `apply` connections #10881

alafanechere · 2022-03-05T22:42:17Z

What

We want to be able to apply YAML configuration for connections.

How

Create Connection resource inheriting from BaseResource.
The factory will do its magic, no edit should be required in command.py

marcosmarxm · 2022-03-07T12:52:57Z

octavia-cli/octavia_cli/apply/resources.py

+            self.source_id,
+            self.destination_id,
+            self.status,


minor but add all param names when call the function

marcosmarxm · 2022-03-07T12:57:00Z

octavia-cli/octavia_cli/apply/resources.py

+            namespace_format=self.namespace_format,
+            prefix=self.prefix,
+            operations_ids=self.operation_ids,
+            sync_catalog=self.sync_catalog,


Are you waiting to receive the sync catalog or is it possible to send a dict?

I'm not sure yet, I'd like to test with a generated config to check if the API client is flexible enough.

marcosmarxm

Overall LGTM if works with the simple call to the API is done, if not need to review again.

marcosmarxm · 2022-03-08T01:28:30Z

octavia-cli/octavia_cli/apply/resources.py

@@ -414,13 +494,15 @@ def factory(api_client: airbyte_api_client.ApiClient, workspace_id: str, configu
        NotImplementedError: Raised if the definition type found in the YAML is not a supported resource.

    Returns:
-        Union[Source, Destination]: The resource object created from the YAML config.
+        Union[Source, Destination, Connection]: The resource object created from the YAML config.
    """
    with open(configuration_path, "r") as f:
        local_configuration = yaml.load(f, yaml.FullLoader)
    if local_configuration["definition_type"] == "source":


Suggested change

if local_configuration["definition_type"] == "source":

if local_configuration["definition_type"] == DEFINITION_TYPE.source:

wdyt creating a enum class for definitions? I think there is other places to be used.
only small suggestion though

This is a future improvement we could make for this file but also in octavia_cli/generate/definitions.py

marcosmarxm · 2022-03-09T13:12:10Z

octavia-cli/octavia_cli/apply/resources.py

+    def _get_comparable_configuration(
+        self,
+    ):  # pragma: no cover
+        if not self.was_created:
+            raise NonExistingResourceError("Can't find a comparable configuration as the remote resource does not exists.")
+        else:
+            return copy.deepcopy(self.remote_resource)
+
    @property
    def was_created(self):
        return True if self.remote_resource else False


I saw other places where the self.was_created is invoked and throw errors. What do you think doing this directly in the was_created function?

Do you mean raising the error in was_created? I'd like to use was_created as a boolean to know if the resource was created or not, I don't want it to throw an error if the resource was not created but simply return false. But maybe you meant running copy.deepcopy(self.remote_resource) in was_created? 🤔

No, only the error handling in the was_created because is also invoked in connection generation command too

We could maybe implement something like self.remote_resource.raise_if_not_created() ?

marcosmarxm · 2022-03-09T13:19:51Z

octavia-cli/octavia_cli/apply/resources.py

+    CHECK_RETURN_TYPE = True
+    APPLY_PRIORITY = 0


Could you add comments about what those two variables are used for?

marcosmarxm · 2022-03-09T13:26:01Z

octavia-cli/octavia_cli/apply/resources.py

+    def _get_comparable_configuration(self):
+        """Get the object to which local configuration will be compared to.
+
+        Returns:
+            dict: Remote source configuration.
+        """
+        comparable_configuration = super()._get_comparable_configuration()
+        return comparable_configuration.connection_configuration


This can't be done in BaseResource and only be overwritten by Connection object?

I duplicated this on purpose as this is really specific to source and destination and not something common to the other potential resources. I was thinking of adding a parent class for Source and Destination (SourceOrDestination(BaseResource) 🤔 ) but thought it was too much for this PR.

marcosmarxm · 2022-03-09T13:29:37Z

octavia-cli/octavia_cli/apply/resources.py

+            prefix=self.configuration["prefix"],
+            schedule=self.configuration["schedule"],
+            resource_requirements=self.configuration["resourceRequirements"],
+            _check_type=False,


Suggested change

_check_type=False,

_check_type=self.CHECK_RETURN_TYPE,

_check_type=False:We disable the validation of type at model instanciation.
_check_return_type=False:We disable the validation of type of the API response.

I made a bit of refacto to make it a bit more readable: 6ec2dd4

octavia-cli/octavia_cli/apply/resources.py

marcosmarxm · 2022-03-09T13:43:04Z

octavia-cli/octavia_cli/apply/resources.py

+        """
+        comparable_configuration = super()._get_comparable_configuration()
+        comparable_configuration.pop("connectionId")
+        comparable_configuration.pop("operationIds")


Today octavia-cli can't work with normalization right?

Exactly, I remove the operationIds in case the user change the transformation in the UI, so we exclude operationIds from the diff computation.

add connection resource

d8da27e

alafanechere temporarily deployed to more-secrets March 5, 2022 22:43 Inactive

alafanechere marked this pull request as ready for review March 5, 2022 22:45

alafanechere requested a review from marcosmarxm March 5, 2022 22:45

update readme

c05c684

alafanechere temporarily deployed to more-secrets March 5, 2022 22:51 Inactive

alafanechere added this to the octavia-cli-alpha milestone Mar 5, 2022

alafanechere added the area/octavia-cli label Mar 5, 2022

alafanechere mentioned this pull request Mar 6, 2022

[EPIC]: Octavia CLI alpha #10704

Closed

marcosmarxm reviewed Mar 7, 2022

View reviewed changes

marcosmarxm requested changes Mar 7, 2022

View reviewed changes

marcosmarxm requested changes Mar 8, 2022

View reviewed changes

alafanechere added 2 commits March 8, 2022 14:45

wip

120563f

test

0e67233

alafanechere temporarily deployed to more-secrets March 8, 2022 19:01 Inactive

alafanechere requested a review from marcosmarxm March 9, 2022 07:33

Merge branch 'master' into augustin/octavia-cli/apply-connection

71381d4

marcosmarxm reviewed Mar 9, 2022

View reviewed changes

alafanechere temporarily deployed to more-secrets March 9, 2022 13:13 Inactive

marcosmarxm reviewed Mar 9, 2022

View reviewed changes

marcosmarxm requested changes Mar 9, 2022

View reviewed changes

octavia-cli/octavia_cli/apply/resources.py Show resolved Hide resolved

octavia-cli/octavia_cli/apply/resources.py Show resolved Hide resolved

marcosmarxm reviewed Mar 9, 2022

View reviewed changes

more readbable implementation for _check_return_type

6ec2dd4

alafanechere temporarily deployed to more-secrets March 9, 2022 14:10 Inactive

more readbable implementation for _check_return_type

f2f6445

alafanechere temporarily deployed to more-secrets March 9, 2022 14:14 Inactive

marcosmarxm approved these changes Mar 9, 2022

View reviewed changes

alafanechere merged commit 56bf982 into master Mar 9, 2022

alafanechere deleted the augustin/octavia-cli/apply-connection branch March 9, 2022 16:08

octavia-squidington-iii mentioned this pull request Mar 9, 2022

Bump Airbyte version from 0.35.47-alpha to 0.35.48-alpha #11007

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐙 octavia-cli: `apply` connections #10881

🐙 octavia-cli: `apply` connections #10881

alafanechere commented Mar 5, 2022 •

edited

Loading

marcosmarxm Mar 7, 2022

marcosmarxm Mar 7, 2022

alafanechere Mar 7, 2022

marcosmarxm left a comment

marcosmarxm Mar 8, 2022

alafanechere Mar 8, 2022 •

edited

Loading

marcosmarxm Mar 9, 2022

alafanechere Mar 9, 2022

marcosmarxm Mar 9, 2022

alafanechere Mar 9, 2022

marcosmarxm Mar 9, 2022

marcosmarxm Mar 9, 2022

alafanechere Mar 9, 2022 •

edited

Loading

marcosmarxm Mar 9, 2022

alafanechere Mar 9, 2022

alafanechere Mar 9, 2022

marcosmarxm Mar 9, 2022

alafanechere Mar 9, 2022

	if local_configuration["definition_type"] == "source":
	if local_configuration["definition_type"] == DEFINITION_TYPE.source:

🐙 octavia-cli: apply connections #10881

🐙 octavia-cli: apply connections #10881

Conversation

alafanechere commented Mar 5, 2022 • edited Loading

What

How

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcosmarxm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alafanechere Mar 8, 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

Choose a reason for hiding this comment

alafanechere Mar 9, 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

🐙 octavia-cli: `apply` connections #10881

🐙 octavia-cli: `apply` connections #10881

alafanechere commented Mar 5, 2022 •

edited

Loading

alafanechere Mar 8, 2022 •

edited

Loading

alafanechere Mar 9, 2022 •

edited

Loading