Skip to content

actionRules - AlertsManagement#4955

Merged
praries880 merged 9 commits intoAzure:masterfrom
roramc:master
Feb 7, 2019
Merged

actionRules - AlertsManagement#4955
praries880 merged 9 commits intoAzure:masterfrom
roramc:master

Conversation

@roramc
Copy link
Contributor

@roramc roramc commented Dec 20, 2018

adding new resource type actionRules in AlertsManagement RP

Latest improvements:

MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.

Contribution checklist:

  • I have reviewed the documentation for the workflow.
  • Validation tools were run on swagger spec(s) and have all been fixed in this PR.
  • The OpenAPI Hub was used for checking validation status and next steps.

ARM API Review Checklist

  • Service team MUST add the "WaitForARMFeedback" label if the management plane API changes fall into one of the below categories.
  • adding/removing APIs.
  • adding/removing properties.
  • adding/removing API-version.
  • adding a new service in Azure.

Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs.

  • If you are blocked on ARM review and want to get the PR merged urgently, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.
    Please follow the link to find more details on API review process.

adding new resource type actionRules in AlertsManagement RP
@openapi-portal-comment
Copy link

If you're a MSFT employee, click this link
to view this PR's validation status on our new OpenAPI Hub spec management tool.

@azuresdkci
Copy link
Contributor

Hello!!

The Rest API Specs team wishes everyone a happy holiday and would like to kindly inform you that responses and review to Pull request will be delayed during the holiday period (now -> 2nds of January) to allow for Awesome reviewers to have an awesome time with their families during the holidays!

Thanks and Have a WONDERFUL HOLIDAY

@AutorestCI
Copy link

AutorestCI commented Dec 20, 2018

Automation for azure-sdk-for-js

Nothing to generate for azure-sdk-for-js

@AutorestCI
Copy link

AutorestCI commented Dec 20, 2018

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

@AutorestCI
Copy link

AutorestCI commented Dec 20, 2018

Automation for azure-sdk-for-python

Nothing to generate for azure-sdk-for-python

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@AutorestCI
Copy link

AutorestCI commented Dec 20, 2018

Automation for azure-sdk-for-go

Nothing to generate for azure-sdk-for-go

"Monthly"
],
"x-ms-enum": {
"name": "SupressionType",
Copy link
Contributor

Choose a reason for hiding this comment

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

SupressionType -> SuppressionType

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved in latest commit

},
"recurrenceValues": {
"type": "array",
"description": "Specifies the values for reccurrence pattern",
Copy link
Contributor

Choose a reason for hiding this comment

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

reccurrence -> recurrence

@AutorestCI
Copy link

AutorestCI commented Dec 20, 2018

Automation for azure-sdk-for-node

Nothing to generate for azure-sdk-for-node

@AutorestCI
Copy link

AutorestCI commented Dec 20, 2018

Automation for azure-sdk-for-java

Nothing to generate for azure-sdk-for-java

Spelling correction and JSON update for delete example
fixed semantic error - added recurrenceType in suppression config
@praries880
Copy link

@roramc The name contains "privatepreview".. if this is indeed a private preview that why not use the private repo to drive this?

@roramc
Copy link
Contributor Author

roramc commented Jan 3, 2019

@roramc The name contains "privatepreview".. if this is indeed a private preview that why not use the private repo to drive this?

As per last discussion, we were asked to push everything into public repo unless it is something where customers should not know about this feature. And also, that way it would be easy to move it to stable version post preview.

@praries880 praries880 added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Jan 7, 2019
code review comments
@praries880
Copy link

@roramc waiting on ARM review before I can approve the PR.

Copy link
Contributor

@ravbhatnagar ravbhatnagar left a comment

Choose a reason for hiding this comment

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

Only reviewed actionRule APIs since that seemed the new type that was added. Next time, please remember to make the first commit which is a copy of existing version. The new changes should come in the next commit onwards. It will make reviewing easier.

"type": "string",
"description": "Description of action rule"
},
"resourceGroup": {
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this needed? its already part of the URL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same object gets returned even when we make a GET call to list all action rules for a given subscription. At that point user would want to know what resource group this action rule is stored in. So we added this in the properties explicitly. If conveying it through Id is enough, we could remove this property as well. Not sure what is the convention.

"description": "Last updated time of action rule. Date-Time in ISO-8601 format.",
"readOnly": true
},
"createdUserName": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this kind of PII is ok to have on the resource response. I will double check on my side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally we would want to show who created/updated the given resource. Aren't other RPs doing the same? Do let us know if there is any concern with this.

Changed enabled flag to status enum
added location as mandatory parameter
@praries880
Copy link

@ravbhatnagar do the latest changess from @roramc address all the issues you raised?

@ravbhatnagar
Copy link
Contributor

awaiting confirmation from service team. We recommended service team check with privacy since it contains potentially PII information.

changed "createdDateTime"/"createdUserName" to "createdAt"/"createdBy"
changed "lastModifiedDateTime"/"lastModifiedUserName" to "lastModifiedAt"/"lastModifiedBy"
Updated conditions to take in operator
Copy link
Contributor

@KrisBash KrisBash left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. Signing off

@KrisBash KrisBash added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed ARMChangesRequested WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Feb 6, 2019
new line at end of file
new line at end of file
@praries880 praries880 merged commit 1e85a38 into Azure:master Feb 7, 2019
@roramc roramc mentioned this pull request Apr 23, 2019
5 tasks
mccleanp pushed a commit that referenced this pull request Mar 23, 2022
* Swagger Specs for SAP Virtual Instance Workload.

* Updates.

* Updates.

* updates.

* Updates.

* Updates.

* Updates.

* Fixes.

Co-authored-by: Ajay Gupta <ajgupt@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants