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

Adding managed-identity support for Azure #1330

Merged

Conversation

FlorianWeissDev
Copy link
Contributor

Description of Change

Adding case for the AzureCredentialsProvider to use DefaultAzureCredential in case of MANAGED_IDENTITY.

Checklist

  • PR description included and stakeholders cc'd
  • tests are changed or added
  • yarn test passes
  • yarn lint has been run
  • git pre-commit hook is successfully executed.
    (Please refrain from using --no-verify)
  • yarn changeset was run and relevant packages have been updated as a major/minor/patch bump with descriptions
    (Determine version update using Semantic Versioning)
  • relevant documentation is changed or added

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

@FlorianWeissDev FlorianWeissDev requested a review from a team as a code owner April 9, 2024 12:45
Copy link
Collaborator

@ccasher ccasher 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! Thank you, @FlorianWeissDev!

@ccasher
Copy link
Collaborator

ccasher commented Apr 15, 2024

Copy link
Collaborator

@ccasher ccasher left a comment

Choose a reason for hiding this comment

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

Linting issues

@FlorianWeissDev
Copy link
Contributor Author

Just need to get the linting error fixed: https://github.com/cloud-carbon-footprint/cloud-carbon-footprint/actions/runs/8615922572/job/23834960524?pr=1330#step:9:23

Thank you for the review. Apologies for the delayed response, I'm on it.

@FlorianWeissDev
Copy link
Contributor Author

FlorianWeissDev commented Apr 22, 2024

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

@FlorianWeissDev
Copy link
Contributor Author

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?

@4upz
Copy link
Member

4upz commented May 24, 2024

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

@4upz
Copy link
Member

4upz commented May 24, 2024

@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?

  • Confirming which version of node you were running when you encountered the heap issue on trunk
  • Reverting the client changes that you made in this PR. Since you only added Azure functionality, you shouldn't be encountering client package linting errors. This could've been a mistake due to your branch being slightly behind trunk.

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.

@FlorianWeissDev
Copy link
Contributor Author

FlorianWeissDev commented May 24, 2024

Good morning @4upz, thank you for your response and no worries about the delay. About your questions:

  • I've been using Node 20.12.2 locally and I'm pretty sure I've not updated since working on this PR. To be sure I've just updated to the latest LTS (20.13.1) and I'm gonna check if I still encounter the issues on the updated trunk branch.
  • Sure, I will revert the linting changes that are not Azure related. Probably later today.

I will give you an update later.

@FlorianWeissDev
Copy link
Contributor Author

@4upz okay, the tests are now running fine for me as well.

  • Updated node to 20.13.1 locally, but 20.12.2 also works fine.
  • Rebased onto trunk.
  • Removed the client package linting changes.

I've not pushed the rebased version of the branch yet, as you are still working on it as well.

@FlorianWeissDev
Copy link
Contributor Author

FlorianWeissDev commented May 24, 2024

Linting is green, too.

@4upz
Copy link
Member

4upz commented May 24, 2024

@4upz okay, the tests are now running fine for me as well.

  • Updated node to 20.13.1 locally, but 20.12.2 also works fine.
  • Rebased onto trunk.
  • Removed the client package linting changes.

I've not pushed the rebased version of the branch yet, as you are still working on it as well.

Thanks for confirming @FlorianWeissDev! You're good to push up your changes. The trunk and CI is stable now

@FlorianWeissDev
Copy link
Contributor Author

FlorianWeissDev commented May 24, 2024

Thanks for confirming @FlorianWeissDev! You're good to push up your changes. The trunk and CI is stable now

@4upz I rebased the branch onto trunk again and pushed the reverted changes. Tests and linting are green.

@4upz
Copy link
Member

4upz commented May 24, 2024

Thanks for confirming @FlorianWeissDev! You're good to push up your changes. The trunk and CI is stable now

@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 4upz merged commit 1415423 into cloud-carbon-footprint:trunk May 24, 2024
1 check failed
@FlorianWeissDev
Copy link
Contributor Author

@4upz thank you for your assistance today!

@FlorianWeissDev FlorianWeissDev deleted the add-azure-managed-role-case branch May 24, 2024 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants