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.

Sign up for GitHub

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

Jump to bottom

Loadfile should also check for conn_type of conn_id provided #1471

Closed
sunank200 opened this issue Dec 20, 2022 · 0 comments · Fixed by #1478
Closed

Loadfile should also check for conn_type of conn_id provided #1471

sunank200 opened this issue Dec 20, 2022 · 0 comments · Fixed by #1478
Assignees
@sunank200
Labels
priority/high High priority product/python-sdk Label describing products
Milestone
1.4.0

Comments

@sunank200
Copy link
Contributor

Describe the bug
There is a issue with the test and I am looking into this. The following task:

aql.load_file(
        input_file=File(
            "s3://astro-sdk/python_sdk/example_dags/data/sample_pattern.csv",
            conn_id="bigquery",
            filetype=FileType.CSV,
        ),
        output_table=Table(conn_id="bigquery", metadata=Metadata(schema="astro")),
        use_native_support=False,
    )

When the conn_id is passed (in this case its is passed as bigquery ), it should have failed but it picked the connection from the File path(in this case it took it as aws_conn) instead. This is a bug on loadfile. That is the reason the CI was passing the above tests and it went unnoticed.

More discussion in: https://astronomer.slack.com/archives/C03868KGF2Q/p1671528754572209?thread_ts=1671455842.541749&cid=C03868KGF2Q

When i run this test locally
pytest -s "tests/test_example_dags.py::test_example_dag[example_load_file]"
it passed as well with following warning

<string>:8: UserWarning: AWS Connection (conn_id='bigquery', conn_type='bigquery') expected connection type 'aws', got 'bigquery'. This connection might not work correctly. Please use Amazon Web Services Connection type.

Expected behavior

  • This should raise exception and fail the task.
@sunank200 sunank200 changed the title Loadfile should also check for conn_type of Loadfile should also check for conn_type of conn_id provided Dec 20, 2022
@phanikumv phanikumv added the product/python-sdk Label describing products label Dec 20, 2022
@phanikumv phanikumv added this to the 1.4.0 milestone Dec 20, 2022
@phanikumv phanikumv added the priority/high High priority label Dec 20, 2022
@sunank200 sunank200 mentioned this issue Dec 21, 2022
Merged
2 tasks
sunank200 added a commit that referenced this issue Dec 28, 2022
@sunank200 @kaxil
Get the file location based on path and conn_id (#1478)
43a4236 
# Description
## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->
Currently `create_file_location` is picking module path based on
`filetype.value` even tough the `conn_id` is passed.
https://github.com/astronomer/astro-sdk/blob/main/python-sdk/src/astro/files/locations/__init__.py#L23
get_location_type doesn't take care of fetching the filelocation based
on `conn_type` as per `conn_id` at all. It just picks up from the path.

<!--
Issues are required for both bug fixes and features.
Reference it using one of the following:

closes: #ISSUE
related: #ISSUE
-->
closes: #1471 


## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->
- Add `validate_conn()` in BaseFilelocation class to match file path and
connection type

## Does this introduce a breaking change?
No

### Checklist
- [x] Created tests which fail without the change (if possible)
- [x] Extended the README / documentation, if necessary

Co-authored-by: Kaxil Naik <kaxilnaik@gmail.com>
sunank200 added a commit that referenced this issue Dec 29, 2022
@sunank200 @kaxil
Get the file location based on path and conn_id (#1478)
91a8b86 
# Description
## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->
Currently `create_file_location` is picking module path based on
`filetype.value` even tough the `conn_id` is passed.
https://github.com/astronomer/astro-sdk/blob/main/python-sdk/src/astro/files/locations/__init__.py#L23
get_location_type doesn't take care of fetching the filelocation based
on `conn_type` as per `conn_id` at all. It just picks up from the path.

<!--
Issues are required for both bug fixes and features.
Reference it using one of the following:

closes: #ISSUE
related: #ISSUE
-->
closes: #1471 


## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->
- Add `validate_conn()` in BaseFilelocation class to match file path and
connection type

## Does this introduce a breaking change?
No

### Checklist
- [x] Created tests which fail without the change (if possible)
- [x] Extended the README / documentation, if necessary

Co-authored-by: Kaxil Naik <kaxilnaik@gmail.com>
utkarsharma2 pushed a commit that referenced this issue Jan 17, 2023
@sunank200 @kaxil @utkarsharma2
Get the file location based on path and conn_id (#1478)
04bbe5e 
# Description
## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->
Currently `create_file_location` is picking module path based on
`filetype.value` even tough the `conn_id` is passed.
https://github.com/astronomer/astro-sdk/blob/main/python-sdk/src/astro/files/locations/__init__.py#L23
get_location_type doesn't take care of fetching the filelocation based
on `conn_type` as per `conn_id` at all. It just picks up from the path.

<!--
Issues are required for both bug fixes and features.
Reference it using one of the following:

closes: #ISSUE
related: #ISSUE
-->
closes: #1471 


## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->
- Add `validate_conn()` in BaseFilelocation class to match file path and
connection type

## Does this introduce a breaking change?
No

### Checklist
- [x] Created tests which fail without the change (if possible)
- [x] Extended the README / documentation, if necessary

Co-authored-by: Kaxil Naik <kaxilnaik@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sunank200 sunank200

Labels
priority/high High priority product/python-sdk Label describing products
Projects
None yet
Milestone
1.4.0
Development

Successfully merging a pull request may close this issue.

Get the file location based on path and conn_id astronomer/astro-sdk
2 participants
@sunank200 @phanikumv