Skip to content

Conversation

@Lee-W
Copy link
Member

@Lee-W Lee-W commented Aug 16, 2023

This PR depends on #33687


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you post a screenshot of a task working successfully with the DefaultAzureCredential that you introduced

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. I'll test and post a screenshot before I mark these PRs as read to review

@Lee-W Lee-W force-pushed the add-DefaultAzureCredential-support-to-data_lake branch 2 times, most recently from bb709df to edf4d99 Compare August 21, 2023 10:35
@Lee-W Lee-W force-pushed the add-DefaultAzureCredential-support-to-data_lake branch from edf4d99 to ea7306f Compare August 24, 2023 05:03
@Lee-W
Copy link
Member Author

Lee-W commented Aug 24, 2023

圖片 圖片

@Lee-W Lee-W marked this pull request as ready for review August 24, 2023 05:06
Copy link
Contributor

@phanikumv phanikumv left a comment

Choose a reason for hiding this comment

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

can you add example DAG for this change and unit tests as well

@Lee-W
Copy link
Member Author

Lee-W commented Aug 24, 2023

can you add example DAG for this change and unit tests as well

I'm using the existing DAG for the testing, and I don't expect code changes to be needed on the DAG. And yes, I'll add some unit tests

@phanikumv
Copy link
Contributor

can you add example DAG for this change and unit tests as well

I'm using the existing DAG for the testing, and I don't expect code changes to be needed on the DAG. And yes, I'll add some unit tests

Does the existing DAG make use of DefaultAzureCredential?

@Lee-W
Copy link
Member Author

Lee-W commented Aug 24, 2023

can you add example DAG for this change and unit tests as well

I'm using the existing DAG for the testing, and I don't expect code changes to be needed on the DAG. And yes, I'll add some unit tests

Does the existing DAG make use of DefaultAzureCredential?

Whether to use DefaultAzureCredential is decided on the connection setup; if tenant and password are not set, it'll fallback to DefaultAzureCredential

@Lee-W Lee-W force-pushed the add-DefaultAzureCredential-support-to-data_lake branch 3 times, most recently from 8aeae8d to 0845088 Compare August 26, 2023 03:39
@Lee-W
Copy link
Member Author

Lee-W commented Aug 26, 2023

Just add some unit tests

@Lee-W Lee-W force-pushed the add-DefaultAzureCredential-support-to-data_lake branch 2 times, most recently from 6e88267 to 74a2750 Compare August 26, 2023 15:06
@Lee-W Lee-W force-pushed the add-DefaultAzureCredential-support-to-data_lake branch from 74a2750 to ac2be8c Compare August 27, 2023 03:10
@Lee-W Lee-W requested a review from phanikumv August 28, 2023 00:14
@potiuk
Copy link
Member

potiuk commented Aug 28, 2023

@phanikumv -> are you good with it ?

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.

3 participants