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

Destination S3 - connector check ignores the s3_bucket_path #15207

Merged
merged 23 commits into from
Aug 24, 2022
Merged

Destination S3 - connector check ignores the s3_bucket_path #15207

merged 23 commits into from
Aug 24, 2022

Conversation

haithem-souala
Copy link
Contributor

Fix for #15195 & #11871

@CLAassistant
Copy link

CLAassistant commented Aug 2, 2022

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the area/connectors Connector related issues label Aug 2, 2022
@haithem-souala haithem-souala changed the title Destination S3 connector check ignores the s3_bucket_path WIP - Destination S3 connector check ignores the s3_bucket_path Aug 3, 2022
@haithem-souala haithem-souala changed the title WIP - Destination S3 connector check ignores the s3_bucket_path Destination S3 - connector check ignores the s3_bucket_path Aug 3, 2022
@airbytehq airbytehq deleted a comment from haithem-souala Aug 3, 2022
@marcosmarxm
Copy link
Member

@sashaNeshcheret can you review this contribution?

@marcosmarxm marcosmarxm self-assigned this Aug 3, 2022
@marcosmarxm
Copy link
Member

marcosmarxm commented Aug 3, 2022

/test connector=connectors/destination-s3

🕑 connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/2792385510
❌ connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/2792385510
🐛 https://gradle.com/s/usqal4x5btr3c

Build Failed

Test summary info:

Could not find result summary

@haithem-souala
Copy link
Contributor Author

Hey @sashaNeshcheret, could you review this PR please?

@marcosmarxm
Copy link
Member

marcosmarxm commented Aug 11, 2022

/test connector=connectors/destination-s3

🕑 connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/2843210270
✅ connectors/destination-s3 https://github.com/airbytehq/airbyte/actions/runs/2843210270
No Python unittests run

Build Passed

Test summary info:

All Passed

@marcosmarxm
Copy link
Member

@sashaNeshcheret can you do the final approval here?

@sashaNeshcheret
Copy link
Contributor

Hi @haithem-souala, sorry i've not a chance to make review earlier.
I've tested PR and it really covers case when user has just read permissions on s3 and in such case we are trying to create new object(with bucket name) instead of using path that specified in config. From my perspective it can be properly published and merged. Thanks for your work.
CC: @marcosmarxm

@github-actions github-actions bot removed area/api Related to the api area/platform issues related to the platform area/server area/scheduler area/worker Related to worker area/frontend Related to the Airbyte webapp kubernetes labels Aug 24, 2022
@marcosmarxm
Copy link
Member

marcosmarxm commented Aug 24, 2022

/publish connector=connectors/destination-s3

🕑 Publishing the following connectors:
connectors/destination-s3
https://github.com/airbytehq/airbyte/actions/runs/2917700974


Connector Did it publish? Were definitions generated?
connectors/destination-s3

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@marcosmarxm marcosmarxm merged commit 7834eef into airbytehq:master Aug 24, 2022
sophia-wiley pushed a commit that referenced this pull request Aug 25, 2022
)

* fix: use correct var for s3 bucket path

* typo

* fix - use bucket path instead of bucket name

* fix: use correct var for s3 bucket path

* typo

* fix - use bucket path instead of bucket name

* add fix to changelog

* solve conflict

* solve conflict

* fix - use bucket path instead of bucket name

* solve md file conflict

* solve dockerfile conflict

* auto-bump connector version [ci skip]

* add eof

Co-authored-by: Marcos Marx <[email protected]>
Co-authored-by: marcosmarxm <[email protected]>
Co-authored-by: Octavia Squidington III <[email protected]>
rodireich pushed a commit that referenced this pull request Aug 25, 2022
)

* fix: use correct var for s3 bucket path

* typo

* fix - use bucket path instead of bucket name

* fix: use correct var for s3 bucket path

* typo

* fix - use bucket path instead of bucket name

* add fix to changelog

* solve conflict

* solve conflict

* fix - use bucket path instead of bucket name

* solve md file conflict

* solve dockerfile conflict

* auto-bump connector version [ci skip]

* add eof

Co-authored-by: Marcos Marx <[email protected]>
Co-authored-by: marcosmarxm <[email protected]>
Co-authored-by: Octavia Squidington III <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment