-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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 new version for proactive detection config #3224
Conversation
Automation for azure-sdk-for-rubyNothing to generate for azure-sdk-for-ruby |
Automation for azure-sdk-for-pythonNothing to generate for azure-sdk-for-python |
Automation for azure-libraries-for-javaNothing to generate for azure-libraries-for-java |
Automation for azure-sdk-for-nodeA PR has been created for you: |
Automation for azure-sdk-for-goA PR has been created for you based on this PR content. Once this PR will be merged, content will be added to your service PR: |
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.
@aviled there is CI failure - with error message DuplicateModelCollsion
-. This detects invalid SDK situation that we weren’t detecting before: if two files that are in the same “tag” in the Readme defines the same model name Foo, both object have to be exactly (at the comma exactly level) the same. Here we have Resource model defined in webTests_API.json, components_API.json and workbooks_API.json, it seems these 3 definitions are not same. could you fix it?
@aviled can you fix the error reported by CI (see previous comments) ? |
Automation for azure-sdk-for-javaNothing to generate for azure-sdk-for-java |
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.
Please model patch payload correctly as described in the email
Looking in Eric Chong @ericc1103 owner of workbooks_api-json spec to address ARM violation. ARM-Violation: "Properties of a PATCH request body must not be required. PATCH operation: 'Workbook_Update' Model Definition: 'WorkbookResource' Property: 'location'" 'WorkbookResource' model inherits from ‘Resource’ (for which we marked location as required), this model is used for both PATCH and PUT. It is required that all the properties of model representing PATCH operation has to be optional. This also indirectly means service should not enforce any requiredness in the PATCH payload. In-order to fix this, we need to define a new model representing Workbook PATCH payload (with all properties optional). The PATCH payload model looks something like WorkbookUpdateParameters as shown below. But please note that only you know what are the PATCH-able properties of workbook so you will need to update (by adding or removing properties) the below model accordingly. "WorkbookUpdateParameters": {
"properties": {
"kind": {
"type": "string",
"description": "The kind of workbook. Choices are user and shared.",
"enum": [
"user",
"shared"
],
"x-ms-enum": {
"name": "SharedTypeKind",
"modelAsString": true
}
},
"tags": {
"type": "object",
"additionalProperties": {
"type": "string"
},
"description": "Resource tags"
},
"properties": {
"x-ms-client-flatten": true,
"description": "Metadata describing a web test for an Azure resource.",
"$ref": "#/definitions/WorkbookPropertiesUpdateParameters"
}
},
"description": "The parameters that can be provided when updating workbook properties properties."
},
"WorkbookPropertiesUpdateParameters": {
"description": "Properties that contain a workbook.",
"properties": {
"name": {
"type": "string",
"description": "The user-defined name of the workbook."
},
"serializedData": {
"type": "string",
"description": "Configuration of this particular workbook. Configuration data is a string containing valid JSON"
},
"version": {
"type": "string",
"description": "This instance's version of the data model. This can change as new features are added that can be marked workbook."
},
"workbookId": {
"type": "string",
"description": "Internally assigned unique id of the workbook definition."
},
"kind": {
"description": "Enum indicating if this workbook definition is owned by a specific user or is shared between all users with access to the Application Insights component.",
"x-ms-client-name": "SharedTypeKind",
"default": "shared",
"type": "string",
"enum": [
"shared",
"user"
],
"x-ms-enum": {
"name": "SharedTypeKind",
"modelAsString": true
}
},
"tags": {
"type": "array",
"items": {
"type": "string"
},
"description": "A list of 0 or more tags that are associated with this workbook definition"
},
"userId": {
"type": "string",
"description": "Unique user id of the specific user that owns this workbook."
},
"sourceResourceId": {
"type": "string",
"description": "Optional resourceId for a source resource."
}
}
}
|
I have updated mine to make PATCH operation of properties as not required. The PR is @ #2950 |
@ericc1103 thanks, your PR is for new preview api-version. The issue here is with 2015-05-01 version of workbooks. I replied to your email and clarified, please take a look. |
"$ref": "#/parameters/ResourceNameParameter" | ||
} | ||
], | ||
"responses": { |
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.
add a default response to capture the error responses. The schema should match the ARm error contract schema. Example can be found in Batch RP swagger.
"readOnly": true, | ||
"description": "Azure resource Id" | ||
}, | ||
"name": { |
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.
this should also be readonly as it comes from the URL
"description": "A ProactiveDetection configuration definition.", | ||
"type": "object", | ||
"x-ms-azure-resource": true, | ||
"properties": { |
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.
"tags" ARM top level property missing? Is this a tracked or proxy resource? Seems tracked since it has a location property
"description": "Properties that define a ProactiveDetection configuration.", | ||
"type": "object", | ||
"properties": { | ||
"Name": { |
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.
how is this name different from the one outside of properties? If different, please call this something else like ruleName
"readOnly": false, | ||
"description": "The rule name" | ||
}, | ||
"Enabled": { |
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.
bools are typically not recommended. String enums generally work better.
"readOnly": false, | ||
"description": "A flag that indicates whether this rule is enabled by the user" | ||
}, | ||
"SendEmailsToSubscriptionOwners": { |
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.
same as above.
"type": "string" | ||
} | ||
}, | ||
"LastUpdatedTime": { |
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.
readonly should be true
} | ||
}, | ||
"paths": { | ||
"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Insights/components/{resourceName}/ProactiveDetectionConfigs": |
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.
Delete API missing. If this is a tracked resource type, patch also MUST be supported atleast for the update of tags but preferably to allow update of any patachable properties.
@aviled any thoughts on @ravbhatnagar 's comments? |
@aviled any updates? If I don't hear back in the next day, we'll be closing the PR and you can reopen it when you're back to it. Thanks! |
Closing due to inactivity, please reopen/comment when you're back to this. Thanks. |
No description provided.