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: list connectors #9546

Merged
merged 11 commits into from
Jan 21, 2022

Conversation

alafanechere
Copy link
Contributor

@alafanechere alafanechere commented Jan 17, 2022

What

We want to display a list of existing sources and destinations connectors that can then be created with (yet to be implemented) octavia create commands.

How

  • Declare two new subcommand groups: octavia > list > connectors
  • Implement sources and destinations commands in the connectors group to list existing sources and destinations in an Airbyte instance. Make use of a Definitions abstract class to
  • Expose the created commands : octavia list connectors sources + octavia list connectors destinations

Recommended reading order

  1. octavia-cli/octavia_cli/entrypoint.py > Addition of list subcommand to octaviacommand group
  2. octavia-cli/octavia_cli/list/commands.py > Declaration of the list and connectors command groups + sources and destinations commands
  3. octavia-cli/octavia_cli/list/definitions.py Implementation of a Definitions abstract class to retrieve and display sources and destinations lists.
  4. octavia-cli/unit_tests/test_list/* write unit tests for the new code.
  5. octavia-cli/unit_tests/test_entrypoint.py update entrypoint test.

🚨 User Impact 🚨

octavia --airbyte-url="https://demo.airbyte.io" list connectors sources and octavia --airbyte-url="https://demo.airbyte.io" list connectors destination are now available commands and display the following output.

Screen Shot 2022-01-17 at 10 30 43

@alafanechere alafanechere temporarily deployed to more-secrets January 17, 2022 08:49 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets January 17, 2022 09:25 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets January 17, 2022 09:35 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets January 17, 2022 09:41 Inactive
@alafanechere alafanechere marked this pull request as ready for review January 17, 2022 09:54
@marcosmarxm
Copy link
Member

@alafanechere these new commands are listing the current version of the containers in the user instance OR the latest version in Docker hub?

Copy link
Member

@marcosmarxm marcosmarxm left a comment

Choose a reason for hiding this comment

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

some initial comments! It's amazing :D

]
return definitions

# TODO alafanechere: declare in a specific formatting module because it will probably be reused
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a base class to control the CLI specification where users can configure too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right. I did not want to make premature optimizations and think that this refactoring need and the right place to put these functions will become more evident during the implementation of other commands.

octavia-cli/octavia_cli/list/definitions.py Outdated Show resolved Hide resolved
@alafanechere alafanechere temporarily deployed to more-secrets January 18, 2022 09:34 Inactive
@alafanechere alafanechere temporarily deployed to more-secrets January 18, 2022 09:46 Inactive
@alafanechere
Copy link
Contributor Author

alafanechere commented Jan 20, 2022

I opened the next PR (draft) that implements a lot of the TODO mentioned in this one and make heavy use of inheritance to have a shared logic between listing commands: #9642

@alafanechere
Copy link
Contributor Author

@alafanechere these new commands are listing the current version of the containers in the user instance OR the latest version in Docker hub?

We are using this endpoint with I think merges GitHub's source_definitions.yaml file and the custom connector.

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.

Looks good to me! Though it looks like there are some build errors that should be fixed before merging

@alafanechere
Copy link
Contributor Author

Thank you @marcosmarxm and @lmossman for the review and your valuable feedback!
The build error is due to some formatting inconsistencies between isort in pre-commit hook and isortFormat task ran by ./gradlew format (line length parameter is not set to 140 in pre-commit). I discarded pre-commit changes for isort in this PR, and will check if we can align formatting task and pre-commit hook parameters in another PR, because it might affect other parts of the project.

@alafanechere alafanechere temporarily deployed to more-secrets January 21, 2022 07:51 Inactive
@alafanechere alafanechere merged commit f6f0c33 into master Jan 21, 2022
@alafanechere alafanechere deleted the augustin/octavia-cli/list-connectors branch January 21, 2022 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants