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

🎉 File source: Add support for Azure blob storage #3660

Merged
merged 7 commits into from
Jun 1, 2021
Merged

🎉 File source: Add support for Azure blob storage #3660

merged 7 commits into from
Jun 1, 2021

Conversation

Phlair
Copy link
Contributor

@Phlair Phlair commented May 27, 2021

What

Adding to Files source connector, ability to grab from Azure Blob Storage

How

smart_open already supports Azure Blob so this was just a case of following existing structure of the Files source connector to create relevant AzBlob functions and integration tests (these required creating a storage account in Airbyte's Azure manually with user integration-test@airbyte.io to use for blob upload/read)

Pre-merge Checklist

  • Run integration tests
  • Publish Docker images

Recommended reading order

  1. test.java
  2. component.ts
  3. the rest

@Phlair
Copy link
Contributor Author

Phlair commented May 27, 2021

I've successfully run the new integration tests locally, however I'm assuming I need to put the new Azure secrets somewhere so that the build can access them for the tests?
(@sherifnada this PR aims to close #1391)

@marcosmarxm marcosmarxm requested review from sherifnada and removed request for masonwheeler and cgardens May 27, 2021 17:43
@sherifnada sherifnada requested a review from davinchia May 28, 2021 02:05
@sherifnada
Copy link
Contributor

sherifnada commented May 28, 2021

@Phlair thank you! review coming shortly. in the meantime for this to run in CI can you add the azblob.json config you're using to Lastpass as a secure note named Source file azure creds?

@davinchia will take care of adding it to github secrets for you where it will be made available in Ci

@Phlair
Copy link
Contributor Author

Phlair commented May 28, 2021

^ those creds are now in Lastpass :)

@davinchia
Copy link
Contributor

davinchia commented May 28, 2021

/test connector=source-file

🕑 source-file https://github.com/airbytehq/airbyte/actions/runs/885079739
❌ source-file https://github.com/airbytehq/airbyte/actions/runs/885079739

Copy link
Contributor

@davinchia davinchia left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks George!

One minor comment. Other than that, I want to wait for the integration test to succeed before merging this in.

@sherifnada
Copy link
Contributor

@Phlair please follow the instructions here #3733 to use the secrets in CI. I've given you permissions on the repo. Then run the test command Davin commented with above to run tests on the branch!

@sherifnada sherifnada changed the title Adding azure blob storage file source connector 🎉 File source: Add support for Azure blob storage May 29, 2021
@davinchia
Copy link
Contributor

davinchia commented May 29, 2021

/test connector=source-file

🕑 source-file https://github.com/airbytehq/airbyte/actions/runs/888404928
✅ source-file https://github.com/airbytehq/airbyte/actions/runs/888404928

@davinchia
Copy link
Contributor

This is passing even though I haven't yet added the secret because the workflow is set up to pull from our repo instead of the forked repo.

@davinchia
Copy link
Contributor

davinchia commented May 29, 2021

/test connector=source-file

🕑 source-file https://github.com/airbytehq/airbyte/actions/runs/888503849
✅ source-file https://github.com/airbytehq/airbyte/actions/runs/888503849

@davinchia
Copy link
Contributor

davinchia commented May 30, 2021

/test connector=source-file

🕑 source-file https://github.com/airbytehq/airbyte/actions/runs/890356896
✅ source-file https://github.com/airbytehq/airbyte/actions/runs/890356896

@davinchia
Copy link
Contributor

davinchia commented May 30, 2021

/test connector=source-file repo=Phlair/airbyte

🕑 source-file https://github.com/airbytehq/airbyte/actions/runs/890357454
❌ source-file https://github.com/airbytehq/airbyte/actions/runs/890357454

@davinchia
Copy link
Contributor

davinchia commented May 31, 2021

@Phlair sorry for the merge delay. I'm also using this PR as a way to refine our tooling around contribution from forked repos. I think I've gotten it mostly working - the current error is due to a bad dependency I fixed in the course of looking at this PR. Do you mind adding this to your changes so I can successfully build this? Happy to do so myself if you give me write permission to your fork!

@Phlair
Copy link
Contributor Author

Phlair commented May 31, 2021

@davinchia No problem, happy to be part of progress 👍
I've merged master of the main repo into my fork, which included your changes. Just started to follow @sherifnada's advice above but looks like the secrets are already added to Github, following commit adds references to these in relevant files.

@Phlair
Copy link
Contributor Author

Phlair commented May 31, 2021

/test connector=source-file repo=Phlair/airbyte

🕑 source-file https://github.com/airbytehq/airbyte/actions/runs/893048880
❌ source-file https://github.com/airbytehq/airbyte/actions/runs/893048880

@Phlair
Copy link
Contributor Author

Phlair commented May 31, 2021

/test connector=source-file repo=Phlair/airbyte

🕑 source-file https://github.com/airbytehq/airbyte/actions/runs/893100706
❌ source-file https://github.com/airbytehq/airbyte/actions/runs/893100706

@Phlair Phlair mentioned this pull request May 31, 2021
2 tasks
@davinchia
Copy link
Contributor

davinchia commented Jun 1, 2021

/test connector=source-file repo=Phlair/airbyte

🕑 source-file https://github.com/airbytehq/airbyte/actions/runs/894557199
✅ source-file https://github.com/airbytehq/airbyte/actions/runs/894557199

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants