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

add managed identity support to azure container registry hook #35320

Conversation

Lee-W
Copy link
Member

@Lee-W Lee-W commented Nov 1, 2023


^ 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.

@Lee-W Lee-W changed the title add managed identity support to azre container registry hook add managed identity support to azure container registry hook Nov 1, 2023
@Lee-W Lee-W force-pushed the add-managed-identity-support-to-container-registry-hook branch from 8164744 to 6077ba7 Compare November 1, 2023 07:10
@Lee-W Lee-W marked this pull request as ready for review November 1, 2023 07:19
@Lee-W Lee-W force-pushed the add-managed-identity-support-to-container-registry-hook branch from 6077ba7 to ef53e26 Compare November 1, 2023 07:54
@Lee-W Lee-W force-pushed the add-managed-identity-support-to-container-registry-hook branch from ef53e26 to 87a2ebc Compare November 1, 2023 08:20
@potiuk
Copy link
Member

potiuk commented Nov 1, 2023

Hey @Lee-W - I merged all the small pull requests for Azure - but I have one kind request after I've done that (actually two):

  1. Is it possible to think about some kind of refactoring that will make all those changes reuse the very similar code of configuration

  2. Is it possible to add unit tests covering all those togeher with this refactor?

That could be a single PR doing all that in one go for all the azure properties with manage identity.

Could you please work on such a PR now that all others are merged ?

@Lee-W
Copy link
Member Author

Lee-W commented Nov 1, 2023

1. Is it possible to think about some kind of refactoring that will make all those changes reuse the very similar code of configuration

Sure. I think I've done the get_defaul_azure_credential part, but the ui part might be something doable as wel..

2. Is it possible to add unit tests covering all those togeher with this refactor?

Sure. I'll improve the coverage in that PR

That could be a single PR doing all that in one go for all the azure properties with manage identity.

Could you please work on such a PR now that all others are merged ?

Should I create a new PR after this is merged? or do you want me to add them in this PR

@potiuk
Copy link
Member

potiuk commented Nov 1, 2023

Should I create a new PR after this is merged? or do you want me to add them in this PR

Aftere that one is fine :)

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.

2 participants