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

AzureSessionProvider changes #695

Merged
merged 1 commit into from
Jun 13, 2024
Merged

Conversation

peterbom
Copy link
Contributor

@peterbom peterbom commented May 23, 2024

Minimal changes for #682:

  • add options for getting session
  • don't require active token for selected tenant

This helps prevent issues like #680 from arising in the future. It also makes the AzureSessionProvider interface more resistant to breakages by adding an Options parameter for session retrieval options that might grow in the future (e.g. to enable specifying extra graph API scopes or application client IDs).

Design decisions:

  1. We will make no immediate attempt to adhere to the structure proposed here.
  2. There is no implicit extra meaning to the selectedTenant property that assumes the user would be able to silently obtain a token for that tenant.

Opening as a draft since those decisions are debatable.

EDIT: I discussed this further with @alexweininger (who authored the Azure authentication extension). Some notes:

  • There are likely to be further changes to the VS Code API to better support the handling of multiple accounts and scopes. We agreed it's not worth making significant changes here until that API is finalized.
  • Our AzureSessionProvider interface currently manages two separate concerns which should probably be distinct:
    1. Authentication state (by account/tenant)
    2. Tenant (and implicitly account) selection (i.e. what account/tenant the user is currently accessing).
      Longer term it would probably make sense to explicitly separate these out. But again, we should wait for further developments in the VS Code API first.

@peterbom peterbom force-pushed the feature/session-provider branch from 4328ba3 to 4853c8d Compare May 28, 2024 20:51
@peterbom peterbom force-pushed the feature/session-provider branch from 4853c8d to 1af470f Compare June 6, 2024 22:05
@peterbom peterbom marked this pull request as ready for review June 10, 2024 21:36
Copy link
Member

@Tatsinnit Tatsinnit left a comment

Choose a reason for hiding this comment

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

🙏 Thanks, looks good to me.

@qpetraroia to be aware and I guess list you are forming to might be better track the co-relation work. Worth to document for chaining the work.

Thanks heaps all.

@peterbom peterbom merged commit c13992c into Azure:main Jun 13, 2024
8 checks passed
tejhan pushed a commit to tejhan/vscode-aks-tools that referenced this pull request Dec 4, 2024
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.

3 participants