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 version 2018-11-27-preview to Microsoft.Insights #4987

Merged

Conversation

aydany
Copy link
Member

@aydany aydany commented Jan 4, 2019

If you are a MSFT employee you can view your work branch via this link.

Contribution checklist:

@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.

@kpajdzik kpajdzik added the review label Jan 4, 2019
@AutorestCI
Copy link

AutorestCI commented Jan 4, 2019

Automation for azure-sdk-for-js

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-js#672

@AutorestCI
Copy link

AutorestCI commented Jan 4, 2019

Automation for azure-sdk-for-python

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-python#2761

@AutorestCI
Copy link

AutorestCI commented Jan 4, 2019

Automation for azure-sdk-for-java

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-java#2139

@AutorestCI
Copy link

AutorestCI commented Jan 4, 2019

Automation for azure-sdk-for-node

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-node#4284

@AutorestCI
Copy link

AutorestCI commented Jan 4, 2019

Automation for azure-sdk-for-ruby

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-ruby#2146

@AutorestCI
Copy link

AutorestCI commented Jan 4, 2019

Automation for azure-sdk-for-go

Nothing to generate for azure-sdk-for-go

@aydany
Copy link
Member Author

aydany commented Jan 4, 2019

This changeset introduces a new extension resource in Microsoft.Insights. This resource is modelled as a singleton collection that provides information about the onboarding status of a resource/resource scope to VM Insights, a component of Azure Monitor.

The new resource has been reviewed with the ARM review board.

This API is not yet enabled in ARM. We were asked to submit this PR and have it approved, before making the manifest changes.

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

The latest package including support for the new API will be generated.
This required several changes:
- The defintion of Resource used by the other specs was incompatible.
This was because the other resources were tracked and the new ones is
proxy. Defined a new ProxyResource type.
- The ErrorResponse definition was incompatible with the rest of the
specs in the folder. This is actually a bug in the exesiting specs. They
do not respect the contract of  { "error": {"code": "", "message": ""}},
i.e., they lack the top-level error element. To work around this issue,
defined a new response type, ResponseWithError, which is compliant
with the latest ARM requirements.
@ravbhatnagar ravbhatnagar added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Jan 7, 2019
@ravbhatnagar
Copy link
Contributor

This was reviewed in person by Vlad and @KrisBash. Kris - please feel free to sign off if it looks good.

@sarangan12
Copy link
Member

Notified @lmazuel @anuchandy @daschult regarding the SDK generation failure and waiting for their update.

@lmazuel
Copy link
Member

lmazuel commented Jan 8, 2019

@AutorestCI rebuild

@lmazuel
Copy link
Member

lmazuel commented Jan 8, 2019

@sarangan12 @aydany I added a commit to the PR to workaround a bug in our CI, because the last commit message contained a "{" character.

@aydany
Copy link
Member Author

aydany commented Jan 8, 2019

@lmazuel Thank you!

@aydany
Copy link
Member Author

aydany commented Jan 9, 2019

@sarangan12 What else do we need to resolve to complete this PR? Thanks!

},
"description": "The onboarding status for the resource. Note that, a higher level scope, e.g., resource group or subscription, is considered onboarded if at least one resource under it is onboarded."
},
"dataStatus": {
Copy link
Contributor

Choose a reason for hiding this comment

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

So, dataStatus is an aggregate in the case of multiple destinations? I.e. if the VM were onboarded to multiple destinations, and only sending to one, it would still be present? Is this enough fidelity or should the status be a property of the destination?

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.

Signing off with comment

@KrisBash KrisBash added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Jan 10, 2019
@sarangan12 sarangan12 merged commit b980271 into Azure:master Jan 14, 2019
TalluriAnusha pushed a commit to AsrOneSdk/azure-rest-api-specs that referenced this pull request Feb 6, 2019
* Adds base for updating Microsoft.Insights from version preview/2018-06-01-preview to version 2018-11-27-preview

* Updates readme

* Updates API version in new specs and examples

* Add initial swagger

Removed some of the autogenerated files and added the swagger file and
examples for the new api.

* Remove extra / from examples

* Fix swagger file

* Improve docs

* Update resource model

Made it the same as the rest of the specs

* Fix the resource data model: no location and tags

* Fix readme

* Generate the latest package

The latest package including support for the new API will be generated.
This required several changes:
- The defintion of Resource used by the other specs was incompatible.
This was because the other resources were tracked and the new ones is
proxy. Defined a new ProxyResource type.
- The ErrorResponse definition was incompatible with the rest of the
specs in the folder. This is actually a bug in the exesiting specs. They
do not respect the contract of  { "error": {"code": "", "message": ""}},
i.e., they lack the top-level error element. To work around this issue,
defined a new response type, ResponseWithError, which is compliant
with the latest ARM requirements.

* Update Python version
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.

8 participants