-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Release playwrighttesting microsoft.playwright testing.auth manager 2024 12 01 #31855
base: main
Are you sure you want to change the base?
Release playwrighttesting microsoft.playwright testing.auth manager 2024 12 01 #31855
Conversation
Copied the files in a separate commit. This allows reviewers to easily diff subsequent changes against the previous spec.
Updated the API version from preview/2023-10-01-preview to stable/2024-12-01.
Next Steps to MergeNext steps that must be taken to merge this PR:
|
...a-plane/Microsoft.PlaywrightTesting.AuthManager/stable/2024-12-01/examples/Accounts_Get.json
Show resolved
Hide resolved
…sting.AuthManager-2024-12-01-devv
…sting.AuthManager-2024-12-01-devv
…sting.AuthManager-2024-12-01-devv
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. 👍
Left one minor comment on eliminating uncommon acronyms.
"/accounts/{accountId}/access-tokens/validate": { | ||
"post": { | ||
"operationId": "AccessTokens_Validate", | ||
"description": "Validates access-token provided in authorization header for the given account id. Authorization required is Bearer JWT Access token provided by MPT service.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and in a few other spots there is the acronym "MPT" -- is this "Microsoft Playwright Service"? It might be better to spell this out.
@doc("Linux OS.") | ||
Linux: "linux", | ||
|
||
@doc("Windows OS.") | ||
Windows: "windows", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker, but we'd expect the casing of enum values be consistent. Here is it lowercase/camelCase e.g. linux
, and the enum value above is PascalCase e.g. Active
. Any reason for this inconsistency?
@path | ||
@doc("The account id.") | ||
accountId: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure how much it effects the Swagger or API. But I'd expect the most important parameter (I assume it be accountId
) at the top.
@doc("When enabled, this feature allows the workspace to upload and display test results, including artifacts like traces and screenshots, in the Playwright portal. This enables faster and more efficient troubleshooting.") | ||
@visibility("read") | ||
reporting?: EnablementStatus = EnablementStatus.Disabled; | ||
reporting?: EnablementStatus = EnablementStatus.Enabled; | ||
|
||
@doc("When enabled, this feature allows the workspace to use local auth (through service access token) for executing operations.") | ||
@visibility("read") | ||
localAuth?: EnablementStatus = EnablementStatus.Disabled; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why there is a service-side default value (e.g. EnablementStatus.Disabled
) to this already read-only property?
A server-side default value is used when user does not define the property in request (e.g. a PUT), and server choose the value. How does this work together with a property user cannot set?
And since the e.g. localAuth?
is an optional property, what does it mean, when the response does not contain localAuth
. Is it "disabled" or "enabled"?
Data Plane API Specification Update Pull Request
Tip
Overwhelmed by all this guidance? See the
Getting help
section at the bottom of this PR description.PR review workflow diagram
Please understand this diagram before proceeding. It explains how to get your PR approved & merged.
API Info: The Basics
Most of the information about your service should be captured in the issue that serves as your API Spec engagement record.
Is this review for (select one):
Change Scope
This section will help us focus on the specific parts of your API that are new or have been modified.
Please share a link to the design document for the new APIs, a link to the previous API Spec document (if applicable), and the root paths that have been updated.
Viewing API changes
For convenient view of the API changes made by this PR, refer to the URLs provided in the table
in the
Generated ApiView
comment added to this PR. You can use ApiView to show API versions diff.Suppressing failures
If one or multiple validation error/warning suppression(s) is detected in your PR, please follow the
Swagger-Suppression-Process
to get approval.
❔Got questions? Need additional info?? We are here to help!
Contact us!
The Azure API Review Board is dedicated to helping you create amazing APIs. You can read about our mission and learn more about our process on our wiki.
Click here for links to tools, specs, guidelines & other good stuff
Tooling
Guidelines & Specifications
Helpful Links
Getting help
write access
per aka.ms/azsdk/access#request-access-to-rest-api-or-sdk-repositoriesNext Steps to Merge
comment. It will appear within few minutes of submitting this PR and will continue to be up-to-date with current PR state.and https://aka.ms/ci-fix.
queued
state, please add a comment with contents/azp run
.This should result in a new comment denoting a
PR validation pipeline
has started and the checks should be updated after few minutes.