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

[S3]Replace AWS CLI configuration and add it in s3.py configuration #1082

Merged
merged 12 commits into from
Jun 26, 2023
Merged

[S3]Replace AWS CLI configuration and add it in s3.py configuration #1082

merged 12 commits into from
Jun 26, 2023

Conversation

akanshi-elastic
Copy link
Contributor

@akanshi-elastic akanshi-elastic commented Jun 16, 2023 •
edited
Loading

Closes https://github.com/elastic/connectors-python/issues/1032###

This PR includes:
1.Add aws keys to connector specific configuration to make it run as native connector

Checklists

Pre-Review Checklist

  • this PR has a meaningful title
  • this PR links to all relevant github issues that it fixes or partially addresses
  • this PR has a thorough description
  • Covered the changes with automated tests
  • Tested the changes locally
  • Added a label for each target release version (example: v7.13.2, v7.14.0, v8.0.0)
  • Contributed any configuration settings changes to the configuration reference

@akanshi-elastic akanshi-elastic marked this pull request as ready for review June 17, 2023 06:40
@akanshi-elastic akanshi-elastic requested a review from a team June 17, 2023 06:40
artem-shelkovnikov
artem-shelkovnikov reviewed Jun 19, 2023
View reviewed changes
connectors/sources/s3.py Outdated
Comment on lines 333 to 339
"region": {
"display": "textarea",
"label": "AWS Region name",
"order": 4,
"type": "str",
"value": "us-east-1",
},
Copy link
Member

Choose a reason for hiding this comment

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

How does it affect the S3 connector?

Does it restrict which buckets can be synced?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were facing some exception related to None type region when try to run the connector without passing region name. However it's not reproducible now and looks like there might be some issue in our s3 configuration that time.

We tested our connector removing the region as you suggested and found that without passing region name our connector is working fine with all type of buckets.
So we can remove the dependency of region & I've updated PR with the same too

@akanshi-elastic
Copy link
Contributor Author

We need to update readme for s3 as well but we don't have permission to update it. Can you please update these 3 points there?
1.From usage remove: Set up the AWS CLI tool
2.Setup everything written under Set up AWS CLI
3.In Confiuration we have add two new fields aws_access_key_id and aws_secret_access_key just after the buckets field

@artem-shelkovnikov
Copy link
Member

cc @danajuratoni - we need to update readme, should we do it ourselves or forward to the tech writers?

@akanshi-elastic akanshi-elastic marked this pull request as draft June 23, 2023 11:12
@akanshi-elastic akanshi-elastic marked this pull request as ready for review June 26, 2023 05:29
@artem-shelkovnikov artem-shelkovnikov enabled auto-merge (squash) June 26, 2023 12:09
@artem-shelkovnikov artem-shelkovnikov merged commit f9d2a01 into elastic:main Jun 26, 2023
@github-actions github-actions bot mentioned this pull request Jun 26, 2023
Merged
github-actions bot pushed a commit that referenced this pull request Jun 26, 2023
@akanshi-elastic
[S3]Replace AWS CLI configuration and add it in s3.py configuration (#…
204b0ad 
…1082)
@github-actions
Copy link

💚 Backport PR(s) successfully created

Status Branch Result
✅ 8.9 #1122

This backport PR will be merged automatically after passing CI.

seanstory pushed a commit that referenced this pull request Jun 26, 2023
@github-actions @akanshi-elastic @artem-shelkovnikov
[S3]Replace AWS CLI configuration and add it in s3.py configuration (#…
007615a 
…1082) (#1122)

Co-authored-by: akanshi-elastic <[email protected]>
Co-authored-by: Artem Shelkovnikov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@artem-shelkovnikov artem-shelkovnikov artem-shelkovnikov approved these changes

Assignees
No one assigned
Labels
auto-backport team:external v8.9.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[S3]Replace AWS CLI configuration and add it in s3.py configuration
2 participants
@akanshi-elastic @artem-shelkovnikov