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

[Identity] Quality improvements #17489

Closed
2 of 6 tasks
sadasant opened this issue Sep 7, 2021 · 2 comments
Closed
2 of 6 tasks

[Identity] Quality improvements #17489

sadasant opened this issue Sep 7, 2021 · 2 comments
Assignees
Labels
Azure.Identity Client This issue points to a problem in the data-plane of the library.

Comments

@sadasant
Copy link
Contributor

sadasant commented Sep 7, 2021

Here’s a list of quality improvements I’d like to do on Identity:

  • Easy cleanups, naming and placement:
    • Think through the private names and re-evaluate improving them. Like the file structure in the MSAL folder. I’m sure some things can feel better if they have a better name. (PR)
    • Go through the available tests and ensure they are ordered correctly. Ensure their names are meaningful, and add comments if needed.
    • Re-review the manual tests and improve their code / comments / docs.
    • Cleanup the folder structure on the manual samples and manual tests. (PR)
  • Concrete changes with net benefits:
    • In a new test file (just one, if possible/if comfortable) write mocked code to trigger all of the known errors the Identity library has, including comments on why these errors may occur. This will both help us keep track of what error cases we have over time, but also will help us see all of our expected errors in a single place (which will make it easier to debug customer issues, write troubleshooting guidelines and see how to improve all of the errors together as a whole, by comparing them to one another.)
  • Potential improvements:
    • Credentials and MSIs are rather similar. In the simplest case, they represent one network request to an endpoint. They also can have synchronous and asynchronous steps to prepare the authentication, they can have a caching policy, a retry policy, and they may or may not use MSAL underneath. In more concrete terms, I feel there’s away to implement the MSAL “flows”, the non-MSAL credentials and the MSIs (Managed Identity Credential scenarios) through a common set of types in a way that it will make it even easier to implement and debug any of those in the future.
@sadasant sadasant added Client This issue points to a problem in the data-plane of the library. Azure.Identity labels Sep 7, 2021
@sadasant sadasant added this to the Backlog milestone Sep 7, 2021
@sadasant sadasant self-assigned this Sep 7, 2021
@sadasant sadasant modified the milestones: Backlog, [2021] November Oct 8, 2021
sadasant added a commit that referenced this issue Nov 6, 2021
This PR contains several cleanups on the Identity project. These include:

- Moved the `test/manual` files to `test/manual/interactive-browser-credential`.
- `samples/manual` to `test/manual/authorization-code-credential`.
- `TokenCredentialOptions` to its own file.
- `ManagedIdentityCredential` cleanups (mostly that I removed the `msiGenericGetToken`).
- Very small cleanups on the credentials (mostly comments).
- MSAL common files to start with "msal" (`msalBrowserCommon`, and `msalNodeCommon`), to make them easier to find. I always had a hard time finding them since they are named differently than the other files in those folders.
- Removed `any` types from the internal implementation of the `ManagedIdentityCredential`.

This PR is part of the work mentioned in #17489 , but it won’t fully solve it.
danieljurek pushed a commit that referenced this issue Nov 10, 2021
This PR contains several cleanups on the Identity project. These include:

- Moved the `test/manual` files to `test/manual/interactive-browser-credential`.
- `samples/manual` to `test/manual/authorization-code-credential`.
- `TokenCredentialOptions` to its own file.
- `ManagedIdentityCredential` cleanups (mostly that I removed the `msiGenericGetToken`).
- Very small cleanups on the credentials (mostly comments).
- MSAL common files to start with "msal" (`msalBrowserCommon`, and `msalNodeCommon`), to make them easier to find. I always had a hard time finding them since they are named differently than the other files in those folders.
- Removed `any` types from the internal implementation of the `ManagedIdentityCredential`.

This PR is part of the work mentioned in #17489 , but it won’t fully solve it.
@sadasant
Copy link
Contributor Author

sadasant commented Dec 16, 2021

Concrete changes ordered by impact:

Here’s a list of quality improvements I’d like to do on Identity:

Task Impact level Impact Cost Done
Simplify the HTTP request mock for Node and the Browser. High Easier to debug tests. 2 No
Run browser tests using CasperJS. High Enables browser testing from node, including navigation. Azure’s interactive login can be mocked. 8 No
Write tests to verify the debugging of DefaultAzureCredential Medium Goes through the review of the debugging capabilities of DefaultAzureCredential and ensures this experience doesn’t deteriorate over time. 3 No
Write tests to verify the debugging of ManagedIdentityCredential Medium Goes through the review of the debugging capabilities of ManagedIdentityCredential and ensures this experience doesn’t deteriorate over time. 3 No
Fix grammar on the test names. Low Easier to debug tests. 1 No

Note

I’m going to drop this one. I think I should revisit it after we do the deliverables of October 2022:

Credentials and MSIs are rather similar. In the simplest case, they represent one network request to an endpoint. They also can have synchronous and asynchronous steps to prepare the authentication, they can have a caching policy, a retry policy, and they may or may not use MSAL underneath. In more concrete terms, I feel there’s away to implement the MSAL “flows”, the non-MSAL credentials and the MSIs (Managed Identity Credential scenarios) through a common set of types in a way that it will make it even easier to implement and debug any of those in the future.

@sadasant
Copy link
Contributor Author

@github-actions github-actions bot locked and limited conversation to collaborators Apr 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Identity Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Development

No branches or pull requests

2 participants