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
Adding managed-identity support for Azure #1330
Adding managed-identity support for Azure #1330
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thank you, @FlorianWeissDev!
Just need to get the linting error fixed: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linting issues
Thank you for the review. Apologies for the delayed response, I'm on it. |
@ccasher I've had a lot of issues with the unit tests locally. I expect, that the pipeline is going to fail in the next step. Is the Google Group the right place to get help to solve the test issues? The failing tests seem to be unrelated to my changes as they are also failing on the trunk branch for me. |
The out of memory error is not caused by the changes in this PR as I can also reproduce it on the trunk. Is this a known issue? |
@FlorianWeissDev sorry for the slow follow-up and roadblocks on this! I see that there's some memory issues with the test that's causing it to fail the checks. I'm currently looking into it to see if I can reproduce. |
@FlorianWeissDev This seems to be a dependency or issue caused by a mismatch of node versions -- especially since the CI pipelines for trunk seem to be intact and I only encounter slow/failing tests when using Node 16/18. I've pushed some fixes to our CI that should allow for me to verify this. In the meantime, would you mind doing the following?
I ran and tested your branch locally and everything seemed fine. I'd be okay with merging in the changes and troubleshooting any test/lint issues ourselves within trunk. |
Good morning @4upz, thank you for your response and no worries about the delay. About your questions:
I will give you an update later. |
@4upz okay, the tests are now running fine for me as well.
I've not pushed the rebased version of the branch yet, as you are still working on it as well. |
Linting is green, too. |
Thanks for confirming @FlorianWeissDev! You're good to push up your changes. The trunk and CI is stable now |
f591285
to
2eea545
Compare
2eea545
to
2e5592a
Compare
@4upz I rebased the branch onto trunk again and pushed the reverted changes. Tests and linting are green. |
Thanks! Everything looks good -- I'll merge it in |
@4upz thank you for your assistance today! |
@FlorianWeissDev No problem, thanks for your patience and for contributing! |
Description of Change
Adding case for the AzureCredentialsProvider to use DefaultAzureCredential in case of MANAGED_IDENTITY.
Checklist
(Please refrain from using
--no-verify
)(Determine version update using Semantic Versioning)
Notes
Same as for PR #1158 this change would only add a test case asserting that a mocked library was called. So we left it out.
Locally I have failing tests but those do not seem to be related to our changes as they are also failing on the trunk branch for me. Would be nice if you could validate the tests as well.
© 2021 Thoughtworks, Inc.
Kind regards,
Florian Weiß
im Auftrag der DB Systel GmbH