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

[CDK] Refactor reading JSON inputs for syncs under one shared method #19733

Closed
3 tasks
brianjlai opened this issue Nov 22, 2022 · 1 comment
Closed
3 tasks
Assignees

Comments

@brianjlai
Copy link
Contributor

brianjlai commented Nov 22, 2022

Tell us about the problem you're trying to solve

This ticket is a precursor to #18946 . We currently attempt to read in JSON file inputs during connector syncs like configs, configured catalogs, and state. Before tackling that issue, we should clean up the code related to file reads and improve code reuse. The naming of the BaseConnector.read_config() is also not accurate since it is currently being used to read other types of inputs too since its a generic json file reader.

Describe the solution you’d like

We should rename BaseConnector.read_config() to read_json_file() to accurately reflect what it is doing. The existing usages of read_config() should be updated.

The Source.read_state() method should start using this newly renamed read_json_file() to read the state JSON file instead of duplicating all the same file reading logic.

Acceptance Criteria

  • When running a check operation on a connector, the operation succeeds with valid json
  • When running a read operation on a connector, the operation succeeds with valid json
  • When running a read where the configured catalog doesn't exist, an error should be thrown
@brianjlai brianjlai added type/enhancement New feature or request needs-triage labels Nov 22, 2022
@maxi297 maxi297 self-assigned this Nov 30, 2022
maxi297 added a commit to maxi297/airbyte that referenced this issue Dec 1, 2022
maxi297 added a commit that referenced this issue Dec 2, 2022
maxi297 added a commit that referenced this issue Dec 2, 2022
maxi297 added a commit that referenced this issue Dec 2, 2022
maxi297 added a commit that referenced this issue Dec 6, 2022
maxi297 added a commit that referenced this issue Dec 6, 2022
maxi297 added a commit that referenced this issue Dec 6, 2022
maxi297 added a commit that referenced this issue Dec 6, 2022
* [ISSUE-19733] clarify error message when reading config files

* [ISSUE #19733] code review and adding validation for spec file as well

* [ISSUE #19733] updating typing of read_json_file

* [ISSUE #19733] fix flake8 error

* [ISSUE #19733] fix linting error

* [ISSUE #19733] remove breaking change

* [ISSUE #19733] bump airbyte cdk version

* [ISSUE #19733] add test for invalid json file on read_state

* [ISSUE #19733] bump version
@maxi297
Copy link
Contributor

maxi297 commented Dec 7, 2022

This has been solved as part of #20019

@maxi297 maxi297 closed this as completed Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants