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

Rename TokenCredential variables to indicate credential used #2950

Merged
merged 1 commit into from
Jun 26, 2023

Conversation

mbarnes
Copy link
Contributor

@mbarnes mbarnes commented Jun 12, 2023

Which issue this PR addresses:

[ARO-1919] AAD: Migrate ARO-RP Azure AD Graph -> Microsoft Graph

What this PR does / why we need it:

Minor follow-up to #1970 where @bennerv requested this in his review.

In most cases the variable rename will be one of:

 fpTokenCredential : ARO first-party application
 spTokenCredential : The cluster's AAD application
msiTokenCredential : Managed service identity for the ARO-RP VM

Test plan for issue:

N/A : Not changing any functionality.

Is there any documentation that needs to be updated for this PR?

No, this is just for code clarity.

@mbarnes mbarnes force-pushed the rename-token-credentials branch 2 times, most recently from 1d6c17b to 8f5b31e Compare June 12, 2023 18:31
@mbarnes mbarnes added the loki label Jun 14, 2023
@github-actions github-actions bot added the needs-rebase branch needs a rebase label Jun 14, 2023
@github-actions
Copy link

Please rebase pull request.

Copy link
Collaborator

@bennerv bennerv left a comment

Choose a reason for hiding this comment

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

just one nit - otherwise lgtm

pkg/env/armhelper.go Outdated Show resolved Hide resolved
In most cases this will be one of:

 fpTokenCredential : ARO first-party application
 spTokenCredential : The cluster's AAD application
msiTokenCredential : Managed service identity for the ARO-RP VM
@mbarnes mbarnes force-pushed the rename-token-credentials branch from 15d85fb to 743d363 Compare June 20, 2023 15:58
Copy link
Contributor

@SrinivasAtmakuri SrinivasAtmakuri left a comment

Choose a reason for hiding this comment

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

LGTM!

@facchettos facchettos merged commit c262800 into Azure:master Jun 26, 2023
@mbarnes mbarnes deleted the rename-token-credentials branch June 26, 2023 17:09
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.

5 participants