Skip to content

Conversation

@ephraimbuddy
Copy link
Contributor

@ephraimbuddy ephraimbuddy commented Sep 8, 2020

This PR adds LocalToAzureDataLakeStorageOperator. This operator will help to add system test for ADLSToGCSOperator.
Also, the upload and download methods of AzureDataLakeHook was improved to accommodate extra arguments since the documentation allows for more arguments.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@ephraimbuddy ephraimbuddy changed the title Add AzureDataLakeUploadOperator Add LocalToAzureDataLakeStorageOperator Sep 8, 2020
@ephraimbuddy
Copy link
Contributor Author

@mik-laj, I plan to work on the missing example dags. This Operator is needed to complete the example dag and system test for adls_to_gcs. It will upload a file to Data Lake which will then be transferred to GCS. Kindly review. Thanks

@mik-laj
Copy link
Member

mik-laj commented Sep 9, 2020

@michalslowikowski00 Can you look at it?

@ephraimbuddy
Copy link
Contributor Author

@mik-laj, @michalslowikowski00, @turbaszek please check if this is necessary :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to pass empty dict in execute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it now, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This empty dictionary is necessary so I could use keyword args for the upload_file method

Copy link
Contributor

Choose a reason for hiding this comment

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

Great. I see that this param is needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

We are trying to avoid full stops in documentation.

Suggested change
are not supported.
are not supported

@michalslowikowski00
Copy link
Contributor

michalslowikowski00 commented Sep 22, 2020

Looks good to me.
Personally I would love to see:

There are no docs about Azure and Microsoft operators at all. You could be a pioneer and add docs and make a good example for future contributors. :)

I am sorry for such late review. :(

@ephraimbuddy
Copy link
Contributor Author

ephraimbuddy commented Sep 22, 2020

Looks good to me.
Personally I would love to see:

There are no docs about Azure and Microsoft operators at all. You could be a pioneer and add docs and make a good example for future contributors. :)

I am sorry for such late review. :(

Yeah, I plan to actively work on example dags on this provider and some others but I'm thinking of starting it up on a separate PR. Currently, there's no SystemTest class for this provider. Do you suggest I start SystemTests with this PR or Create another one and do example dags with tests in separate PRs?

@michalslowikowski00
Copy link
Contributor

michalslowikowski00 commented Sep 22, 2020

Looks good to me.
Personally I would love to see:

There are no docs about Azure and Microsoft operators at all. You could be a pioneer and add docs and make a good example for future contributors. :)
I am sorry for such late review. :(

Yeah, I plan to actively work on example dags on this provider and some others but I'm thinking of starting it up on a separate PR. Currently, there's no SystemTest class for this provider. Do you suggest I start SystemTests with this PR or Create another one and do example dags with tests in separate PRs?

IMHO SystemTest could be in separate PR but example dag should be in this PR. For SystemTest you need example dag anyway.

@ephraimbuddy
Copy link
Contributor Author

Alright. Thanks

@michalslowikowski00
Copy link
Contributor

if you need help, let me know.

@ephraimbuddy
Copy link
Contributor Author

I have added the example dag. Will add a doc and system test to verify the example in a separate PR as discussed @michalslowikowski00

@michalslowikowski00
Copy link
Contributor

I have added the example dag. Will add a doc and system test to verify the example in a separate PR as discussed @michalslowikowski00

Great. Looks good for me.
Now someone from the community has to accept it.

Copy link
Member

Choose a reason for hiding this comment

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

Can we add type annotations, please? 🙏

Copy link
Member

Choose a reason for hiding this comment

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

Same here, otherwise those changes are not related

Copy link
Member

Choose a reason for hiding this comment

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

Can you add a reference to how to guide?

Comment on lines +61 to +69
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest adding those fields to template fields.

@ephraimbuddy
Copy link
Contributor Author

Hi @turbaszek, please can you take a look once more, thanks.

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.

4 participants