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

add Azure Monitor Action Group 2018-09-01 API for armrole receiver #3770

Closed
wants to merge 2 commits into from
Closed

Conversation

RyanD1
Copy link
Contributor

@RyanD1 RyanD1 commented Aug 30, 2018

This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.

PR information

  • The title of the PR is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
  • Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
  • If applicable, the PR references the bug/issue that it fixes.
  • Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

@AutorestCI
Copy link

AutorestCI commented Aug 30, 2018

Automation for azure-sdk-for-python

A 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:
Azure/azure-sdk-for-python#2761

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@AutorestCI
Copy link

AutorestCI commented Aug 30, 2018

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

@AutorestCI
Copy link

AutorestCI commented Aug 30, 2018

Automation for azure-sdk-for-node

A PR has been created for you:
Azure/azure-sdk-for-node#3465

@AutorestCI
Copy link

AutorestCI commented Aug 30, 2018

Automation for azure-sdk-for-java

A 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:
Azure/azure-sdk-for-java#2139

@AutorestCI
Copy link

AutorestCI commented Aug 30, 2018

Automation for azure-sdk-for-go

A PR has been created for you:
Azure/azure-sdk-for-go#2812

@jhendrixMSFT
Copy link
Member

@RyanD1 has this new API version been reviewed by ARM?

@RyanD1
Copy link
Contributor Author

RyanD1 commented Aug 30, 2018

@jhendrixMSFT the change for ARM template schema is still under review, will update in this PR once it goes through.

@jhendrixMSFT jhendrixMSFT added WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required Reassign labels Aug 30, 2018
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.

@RyanD1 - please take a look at these comments. @simongdavies FYI

"200": {
"description": "The receiver was successfully enabled."
},
"409": {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be covered in the default response. So you can remove the 409 response.

"description": "An action group resource.",
"allOf": [
{
"$ref": "#/definitions/Resource"
Copy link
Contributor

Choose a reason for hiding this comment

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

"maxLength": 12,
"description": "The short name of the action group. This will be used in SMS messages."
},
"enabled": {
Copy link
Contributor

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. They are less descriptive and dont allow for future expansion of states. Can you make it a string enum?

"default": true,
"description": "Indicates whether this action group is enabled. If an action group is not enabled, then none of its receivers will receive communications."
},
"emailReceivers": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you consider modeling the receivers as a resource type with a kind property to disambiguate which kind of receiver it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

So this would make the api something like this - .../actionGroups/{groupName}/receivers/{receiverName}.

"type": "string",
"description": "JSON blob for the configurations of the ITSM action. CreateMultipleWorkItems option will be part of this blob as well."
},
"region": {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we make this an enum?

"httpTriggerUrl"
]
},
"ArmRoleReceiver": {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is an armRolereceiver?

}
}
},
"ActionGroupPatchBody": {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would make it easier to update the receivers if Patch supported that. Else, everytime user needs to update some receiver - either add new receivers, or remove a receiver, or update some setting of already added receiver, they will have to re-PUT the entire action group again.

@jhendrixMSFT
Copy link
Member

@RyanD1 any update on ARM's feedback?

@jhendrixMSFT
Copy link
Member

@RyanD1 ping

@jhendrixMSFT
Copy link
Member

Closing due to inactivity. Please reopen when ready to engage.

@AutorestCI
Copy link

AutorestCI commented Sep 24, 2018

Automation for azure-sdk-for-js

Nothing to generate for azure-sdk-for-js

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants