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 AzurePipelinesCredential #2306

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add AzurePipelinesCredential #2306

wants to merge 2 commits into from

Conversation

heaths
Copy link
Member

@heaths heaths commented Mar 8, 2025

Resolves #2030. This includes some small amount of refactoring of existing code that will be necessary sooner than later, but I didn't want to take all that on in this PR focused on AzurePipelinesCredential. Still, I wasn't about to repeat the same antiquated patterns.

@heaths heaths requested a review from RickWinter as a code owner March 8, 2025 05:01
@heaths heaths requested a review from chlowell March 8, 2025 05:01
@heaths heaths requested a review from JeffreyRichter as a code owner March 8, 2025 05:01
@heaths heaths requested a review from jhendrixMSFT March 8, 2025 05:01
@github-actions github-actions bot added Azure.Core The azure_core crate Azure.Identity The azure_identity crate labels Mar 8, 2025
@heaths
Copy link
Member Author

heaths commented Mar 8, 2025

There are some TODOs that require some heavy lifting, like using either the azure_core Pipeline or making an azure_identity-specific one; however, I want to focus on an MVP that enables our live tests since we need to get those up and running ASAP (/cc @hallipr). azure_identity needs a lot of work already including some major refactoring, using more of the pipelining, etc., so we can do all that then and be consistent.

@RickWinter @JeffreyRichter @analogrelay as we start getting more and more nested options, the API is feeling a bit unwieldy. In addition, I'm not crazy about making it obvious we use a ClientAssertionCredential by nesting it's options. It's not that I think we should hide it, but feels like shipping an implementation detail (just like we shouldn't ship our org chart). Guidelines do leave the possibility of builders open, so should we consider that for credentials? Or do we duplicate fields and manage all that as we make changes? It's not terribly hard since Rust forces us to assign fields if we don't use ..Default::default().
I'm open to other suggestions on design, too.

Comment on lines 32 to 34
C: FnMut(&Request) -> Pin<Box<dyn Future<Output = Result<Response>> + Send + Sync + '_>>
+ Send
+ Sync,
Copy link
Member Author

Choose a reason for hiding this comment

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

@analogrelay any other good ideas here? The DX this forces (see test below; we have to pin the future) is unwieldy but I couldn't figure out a better way without taking advantage of Rust 1.85 async captures.

Copy link
Member Author

Choose a reason for hiding this comment

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

I cleaned it up a little using a BoxedFuture, though the boxed() "extension method" probably does more heavy lifting here. Still open to other suggestions, but this seems to be a fairly common solution I found - more than what inspired me originally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Core The azure_core crate Azure.Identity The azure_identity crate
Projects
Status: Untriaged
Development

Successfully merging this pull request may close these issues.

Add AzurePipelineCredential for test authentication
1 participant