[Hub Generated] Review request for Microsoft.Security to add version stable/2020-01-01 #8805
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Pull request contains merge conflicts. |
|
No commit pushedDate could be found for PR 8805 in repo Azure/azure-rest-api-specs |
nitsi
added
the
DoNotMerge
|
Can one of the admins verify this patch? |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
Hi @nitsi is this PR ready for review? If so, given that you have the permission to edit labels, it would be best to add a |
azure-sdk-for-python - Release
- Breaking Change detected in SDK
|
azure-sdk-for-net - Release
|
azure-cli-extensions
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
azure-sdk-for-go - Release
|
azure-sdk-for-js - Release
|
azure-sdk-for-java - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
|
You have inconsistency enum definitions between here and the |
ArcturusZhang
requested changes
Mar 24, 2020
specification/security/resource-manager/Microsoft.Security/stable/2020-01-01/alerts.json
Outdated
Show resolved
Hide resolved
ArcturusZhang
added
WaitForARMFeedback
|
Pull request contains merge conflicts. |
1 similar comment
|
Pull request contains merge conflicts. |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
There is another enum called |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This was referenced Apr 2, 2020
|
Azure Pipelines successfully started running 1 pipeline(s). |
anthony-c-martin
requested changes
Apr 19, 2020
| } | ||
| } | ||
| }, | ||
| "ResourceIdentifier": { |
There was a problem hiding this comment.
Please model resource identifiers using the standard ARM id string format rather than writing a custom model for it. This has the following benefits:
- Consistency with other ARM services.
- Allows other services (ARM, ARG, ARM Deployments) to programatically understand resource dependencies.
- Avoids users of the SDK to have to write resourceId parsing (e.g. if a user wants to fetch a loganalytics resource and pass the id to this field).
There was a problem hiding this comment.
Hi, can you explain " standard ARM id string format" ?
ResourceIdentifiers has 2 formatting types, which property "type" is common , and each has its own additional properties.
Do you mean to include only the type property as string, and allow additional properties?
There was a problem hiding this comment.
I think Anthony is talking about using the standard ARM resource ID string (something like /subscriptions/{id}/resourceGroups/{rg}/providers...) You are basically doing that with Azure Resources, but I definitely see the challenge with LA which is partially in the data plane (due to agent IDs).
In reply to: 437166909 [](ancestors = 437166909)
There was a problem hiding this comment.
We definitely should drop "Identifier" from the definition name and the property above (see comment above). It can be confused with the meaning of the id property which is the resource identifier.
In reply to: 439029078 [](ancestors = 439029078,437166909)
There was a problem hiding this comment.
See my comment on your other comment about the resourceIdentifiers. It is not just the "ARMResourceId" , there are some identifiers for the resource , ARMResourceId is identifier in terms of AzureResource. There are some more resource identifiers for the resource
There was a problem hiding this comment.
Replied on that one. I'm disagreeing with Anthony and agreeing with you, but have a concern about naming.
In reply to: 439064304 [](ancestors = 439064304)
specification/security/resource-manager/Microsoft.Security/stable/2020-01-01/alerts.json
Outdated
Show resolved
Hide resolved
specification/security/resource-manager/Microsoft.Security/stable/2020-01-01/alerts.json
Outdated
Show resolved
Hide resolved
specification/security/resource-manager/Microsoft.Security/stable/2020-01-01/alerts.json
Outdated
Show resolved
Hide resolved
specification/security/resource-manager/Microsoft.Security/stable/2020-01-01/alerts.json
Outdated
Show resolved
Hide resolved
specification/security/resource-manager/Microsoft.Security/stable/2020-01-01/alerts.json
Outdated
Show resolved
Hide resolved
specification/security/resource-manager/Microsoft.Security/stable/2020-01-01/alerts.json
Outdated
Show resolved
Hide resolved
specification/security/resource-manager/Microsoft.Security/stable/2020-01-01/alerts.json
Outdated
Show resolved
Hide resolved
|
Comment was made before the most recent commit for PR 8805 in repo Azure/azure-rest-api-specs |
nitsi
added
WaitForARMFeedback
majastrz
reviewed
Jun 11, 2020
| "resourceIdentifiers": { | ||
| "readOnly": true, | ||
| "type": "array", | ||
| "description": "The resource identifiers for this alert which can be used to direct the alert to the right product exposure group (tenant, workspace, subscription etc.). There can be multiple identifiers of different type per alert.", |
There was a problem hiding this comment.
The resource identifiers for this alert [](start = 26, length = 39)
This doesn't seem accurate. The resource identifier for the alert is in the id property of the alert resource. I think you mean something else here. Would impactedResources, affectedResources or something like that be a better fit?
There was a problem hiding this comment.
It is an array of all the identifiers for the resource: we can have AADIdentifier (which has AadTenantId inside) , AzureResource ( which has AzureResourceId inside) , LogAnalytics ( which has WorkspaceId, WorkspaceSubscriptionId and some more identifiers inside).
So the name - resourceIdentfiers will hold all the identifiers for this resource. not just the "resourceId" itself
There was a problem hiding this comment.
Yes, I got that. All I'm saying is that the description text of "The resource identifiers for this alert" is confusing. The text is ambiguous and implies that the alert is being identified by the resourceIdentifiers property. I think part of the issue is the name of the property (schema of the property is fine) I think the name of the name should be changed to something that better captures the relationship between the alert and the resources. Maybe impactedResources, affectedResources, or something like that?
In reply to: 439062511 [](ancestors = 439062511)
There was a problem hiding this comment.
Changed the description, and removed "for this alert". Do you thing it is more clear now?. We use this name internally in schema which we expose to out partners so changing it might be confusing.
majastrz
added
the
ARMChangesRequested
|
Azure Pipelines successfully started running 1 pipeline(s). |
azure-sdk-for-python-track2 - Release
No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured
|
nitsi
removed
the
ARMChangesRequested
|
@chiragg4u can you take over ARM review on this one also? |
chiragg4u
reviewed
Jun 22, 2020
| "type": "AzureResource" | ||
| }, | ||
| { | ||
| "workspaceId": "f419f624-acad-4d89-b86d-f62fa387f019", |
There was a problem hiding this comment.
Why did we modeled this differently? Can't we use azureResourceId as workspace is also an Azure resource?
Please work with Gaurav Bhatnagar for continuing the review.
There was a problem hiding this comment.
Those are different values , or did you mean something else? I'll work with Gaurav
There was a problem hiding this comment.
can the workspace Id be the ARM resource ID which is of the form /subsccriptions/{id}/resourceGroups/{rgname}/..../workspaces/{name}? @yibirnba ?
There was a problem hiding this comment.
we keep this separately on different properties: WorkspaceId, WorkspaceSubscriptionId, WorkspaceResourceGroup, AgentId , since those are not mandatory fields and we want to show information that we have
| "type": "object", | ||
| "description": "describes security alert properties.", | ||
| "properties": { | ||
| "alertType": { |
There was a problem hiding this comment.
please make this enum and list all the possible values
There was a problem hiding this comment.
alertType is not a closed list, there are hundreds of types, and new types are been added often
| "type": "AzureResource" | ||
| }, | ||
| { | ||
| "workspaceId": "f419f624-acad-4d89-b86d-f62fa387f019", |
There was a problem hiding this comment.
can the workspace Id be the ARM resource ID which is of the form /subsccriptions/{id}/resourceGroups/{rgname}/..../workspaces/{name}? @yibirnba ?
|
Adding new APIs requires a new api-version as per the new versioning guidelines. If you need or think this must be added to the existing api-version, please get sign off from Azure API review board. Adrian hall, Johan Stenberg, Jeffrey Richter etc. can help |
ravbhatnagar
removed
the
WaitForARMFeedback
|
Removing ARM Review label as APIs are fine from ARM standpoint. |
ravbhatnagar
added
the
ARMSignedOff
|
Hi @nitsi is this PR ready to merge? This LGTM and should be good to merge. If this is ready to merge, please remove the |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Hi @nitsi do we have any update on this? Please let me know if this is ready to merge |
Not yet, we still have a few things to do before:
We will give you an update once we are ready |
nitsi
added
Approved-OkToMerge
|
@ArcturusZhang Please marge |
ArcturusZhang
approved these changes
Aug 31, 2020
azure-resource-manager-schemas - Release
|
11 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If you are a MSFT employee you can view your work branch via this link.
Contribution checklist: