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

Check if connection is minio then set use_native_support=False #1767

Merged
merged 25 commits into from
Feb 21, 2023

Conversation

sunank200
Copy link
Contributor

@sunank200 sunank200 commented Feb 15, 2023

Description

What is the current behavior?

When the user passes the s3 url but has minio as the setup, for minio to any database transfers by default it uses the native path and fails as there is no native path. There is no direct way to check if the connection is of type minio. Introducing a parameter minio in extras as part of connections would help to recognize the connection type

closes: #1746

What is the new behavior?

  • Check if the AWS connection has minio in extra then set use_native_support=False
  • Example minio conn:
  - conn_id: minio_conn
    conn_type: aws
    description: null
    extra:
      aws_access_key_id: "dummy"
      aws_secret_access_key: "dummy"
      endpoint_url: "http://127.0.0.1:9000"
      minio: True

Does this introduce a breaking change?

No

Checklist

  • Created tests which fail without the change (if possible)
  • Extended the README / documentation, if necessary

@codecov
Copy link

codecov bot commented Feb 15, 2023

Codecov Report

Base: 92.62% // Head: 93.05% // Increases project coverage by +0.42% 🎉

Coverage data is based on head (09f6b37) compared to base (c92849a).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1767      +/-   ##
==========================================
+ Coverage   92.62%   93.05%   +0.42%     
==========================================
  Files          93       93              
  Lines        4923     4937      +14     
  Branches      484      486       +2     
==========================================
+ Hits         4560     4594      +34     
+ Misses        265      248      -17     
+ Partials       98       95       -3     
Impacted Files Coverage Δ
python-sdk/src/astro/databases/base.py 91.32% <100.00%> (+3.60%) ⬆️
python-sdk/src/astro/sql/operators/load_file.py 93.20% <0.00%> (-3.89%) ⬇️
python-sdk/src/astro/databases/google/bigquery.py 87.94% <0.00%> (-0.45%) ⬇️
...ro/databases/databricks/load_file/load_file_job.py 96.77% <0.00%> (+1.61%) ⬆️
python-sdk/src/astro/sql/__init__.py 96.00% <0.00%> (+4.00%) ⬆️
python-sdk/src/astro/files/locations/amazon/s3.py 92.72% <0.00%> (+5.45%) ⬆️
...hon-sdk/src/astro/files/locations/google/gdrive.py 97.64% <0.00%> (+15.29%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dimberman
Copy link
Collaborator

@sunank200 can you please offer more context in the PR template as to what this PR is doing and why we are doing it? Maybe a bit on how it solves this bug?

@sunank200
Copy link
Contributor Author

sunank200 commented Feb 16, 2023

@sunank200 can you please offer more context in the PR template as to what this PR is doing and why we are doing it? Maybe a bit on how it solves this bug?

sure @dimberman This PR is still in draft but I have updated the PR description

@utkarsharma2 utkarsharma2 merged commit 3610312 into main Feb 21, 2023
@utkarsharma2 utkarsharma2 deleted the minio-fix branch February 21, 2023 12:12
utkarsharma2 added a commit that referenced this pull request Feb 21, 2023
# Description
## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->
When the user passes the s3 url but has minio as the setup, for minio to
any database transfers by default it uses the native path and fails as
there is no native path. There is no direct way to check if the
connection is of type minio. Introducing a parameter `minio` in extras
as part of connections would help to recognize the connection type

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

closes: #ISSUE
related: #ISSUE
-->
closes: #1746

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- Check if the AWS connection has `minio` in `extra` then set
use_native_support=False
- Example minio conn:
```
  - conn_id: minio_conn
    conn_type: aws
    description: null
    extra:
      aws_access_key_id: "dummy"
      aws_secret_access_key: "dummy"
      endpoint_url: "http://127.0.0.1:9000"
      minio: True
```


## 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: utkarsh sharma <utkarsharma2@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Minio support with Snowflake not working
5 participants