Application Insights: CRUD for a new Instance #757

tombuildsstuff · 2016-11-23T05:56:27Z

This is a Work-in-Progress designed to get some early feedback - I've had to try and reverse engineer this.. which was an interesting experience.

Tasks for my list:

Build the JSON
Double-check with the Responses
Validate using the extension
Generate a client to check it works as expected

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

I have read the contribution guidelines.
My spec meets the review criteria:
- The spec conforms to the Swagger 2.0 specification.
- Validation errors from the Linter extension for VS Code have all been fixed for this spec. (Note: for large, previously checked in specs, there will likely be many errors shown. Please contact our team so we can set a timeframe for fixing these errors if your PR is not going to address them).
- The spec follows the patterns described in the Swagger good patterns document unless the service API makes this impossible.

azurecla · 2016-11-23T05:56:41Z

Hi @tombuildsstuff, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.azure.com.

TTYL, AZPRBOT;

tombuildsstuff · 2016-11-23T06:19:32Z

arm-insights/2014-04-01/swagger/insights.json

+                },
+                "x-ms-odata": "#/definitions/Resource"
+            },
+            "post": {


tombuildsstuff · 2016-11-23T06:20:03Z

arm-insights/2014-04-01/swagger/insights.json

+            },
+            "post": {
+                "description": "Creates or Updates an Application Insights instance",
+                "operationId": "Insights_CreateUpdate",


CreateOrUpdate to match the others

tombuildsstuff · 2016-11-23T06:20:25Z

arm-insights/2014-04-01/swagger/insights.json

+                },
+                "kind": {
+                    "type": "string",
+                    "description": "Application Insights Kind"


this wants to be the enum too

tombuildsstuff · 2016-11-23T06:20:49Z

arm-insights/2014-04-01/swagger/insights.json

+                },
+                "applicationId": {
+                    "type": "string",
+                    "description": "XXX"


provided by the user, needs to be unique

amarzavery · 2016-11-23T18:54:06Z

@gucalder - Since you are from the insights team, can you please review this PR?
@tombuildsstuff - We are following a composite model for insights.

Looks like these APIs are not covered in the current specs that are already merged in the master branch.

amarzavery · 2016-11-23T18:52:03Z

arm-insights/2014-04-01/swagger/insights.json

@@ -0,0 +1,342 @@
+{


indentation: 2 spaces please.

tombuildsstuff · 2016-11-23T19:36:51Z

@amarzavery as I'm new to this - I'm guessing that means I just add the file path to that file?

tombuildsstuff · 2016-11-23T19:40:57Z

Is there a command line linter I should be using for this? I'm using Atom and the Swagger Editor to edit this, so I assumed it'd be the default settings - I dont want to install VSCode - so the plugin there is a nonstarter for me :)

tombuildsstuff · 2016-11-27T08:02:19Z

@amarzavery updated this to have an entry in the composite spec / be named descriptively

tombuildsstuff · 2016-12-06T08:24:59Z

This has been open for nearly 2 weeks with no responses

could someone take a look please? :)

amarzavery · 2016-12-06T21:41:28Z

arm-insights/2014-04-01/swagger/insightsManagementClient_Components.json

+          },
+          {
+            "$ref": "#/parameters/NameParameter"
+          }


It is surprising to me that the request does not have a body parameter. If you were to update some properties of the ApplicationInsights instance then what would you update and how would you update?

Perhaps I'm mis-understanding / you've posted this comment in the wrong place in the file - but why would the DELETE HTTP Call need a body to update?

With regards for Updating Redis instances - we're doing this via the CreateOrUpdate method - see here for the implementation within Terraform - this is hooked up and working (generated from before it was turned into a composite swagger)

amarzavery · 2016-12-06T21:46:31Z

@tombuildsstuff - Sorry for the delay in getting this merged. Will give this a higher priority amongst other items.

I was looking online to find the documentation of the REST API and I found this link. I wanted to confirm whether you were referring this doc while designing the swagger spec.

I have been talking to teams internally and they are saying that this spec actually belongs to a different service named application-insights and not Insights. Hence this should reside in a different folder named arm-applicationInsights/<api-version>/swagger/applicationInsights.json.

Please do not make the change yet, I am still talking to them to get more clarity on this. There are lot of changes that need to be made to this spec. I have made the changes locally. Please take a look at the attached file to see the difference.
ai.json.txt

tombuildsstuff · 2016-12-07T08:41:13Z

@amarzavery I ended up reverse-engineering the API, so I wasn't aware of the docs.

tombuildsstuff · 2016-12-12T20:24:32Z

@amarzavery 👋 did you hear back from the team by any chance?

I don't wish to be rude - but given this has been open for 3 weeks now (and doesn't look like it's going anywhere anytime soon) - am I better off just implementing support directly in Go? :(

salameer · 2016-12-12T23:23:03Z

Hello @tombuildsstuff, we would really like to apologize for the delay on response from our side, and we're currently actively looking for the right party that'll be able to review this spec and provide feedback if any before making a decision on the merge.

btw your not being rude at all :) and we really appreciate all of your efforts and contributions on this repo and others!

SergeyKanzhelev · 2017-01-04T21:13:23Z

Some feedback:

Need to describe pricing parameters when creating a component.
Do not call it application insights instance, call component everywhere

See some descriptions in here that can be re-used: https://docs.microsoft.com/en-us/azure/application-insights/app-insights-powershell#create-an-azure-resource-manager-template

d3r3kk · 2017-01-06T00:27:34Z

Hello all, I'm from the Application Insights team and I've been asked to look into this issue and to help provide resolution to this PR.

Thanks so much @tombuildsstuff for providing the initial work and @amarzavery for adding your changes. I will likely have questions for you two as I gain insight on this PR. Also, I'll likely touch base with @SergeyKanzhelev and @salameer too just to make sure I cover all the bases!

Not 100% certain how much time I'll need to ramp up, but I feel it should take me until next week to drive us to the solution we'll adopt going forward. Please feel free to reach out to me if you have any concerns or questions!

d3r3kk · 2017-01-26T22:16:25Z

Unfortunately higher priority work has gotten in my way, and has hampered me from spending the time I need to get this work done in the original time I planned. This swagger specification is the next thing on my plate however, and will remain so until my highest priority item resolves.

I won't be able to supply an update until the week of Feb 6th at best, from what I can tell so far.

Again, please feel free to send me messages/emails if you wish to discuss this effort further.

salameer · 2017-03-17T01:38:11Z

Closing this PR since we have PR #988 opened by the application insights teams. @tombuildsstuff please let me know if that's good or reopen otherwise :)

tombuildsstuff · 2017-03-17T13:45:23Z

thanks @salameer

First-stab at the Application Insights Create/Update/Get/Delete schema

583d6dd

azurecla added the cla-required label Nov 23, 2016

tombuildsstuff commented Nov 23, 2016

View reviewed changes

tombuildsstuff mentioned this pull request Nov 23, 2016

Insights API Status? #505

Closed

tombuildsstuff added 2 commits November 23, 2016 14:25

Added msising values for some fields

bd5b65f

JSON only / PUT for Create&Update

c5af5ab

tombuildsstuff changed the title ~~[WIP] Application Insights: CRUD for a new Instance~~ Application Insights: CRUD for a new Instance Nov 23, 2016

tombuildsstuff mentioned this pull request Nov 23, 2016

Generated the Application Insights SDK Azure/azure-sdk-for-go#473

Closed

amarzavery reviewed Nov 23, 2016

View reviewed changes

tombuildsstuff mentioned this pull request Nov 23, 2016

[WIP] provider/azurerm: Application Insights hashicorp/terraform#10326

Closed

6 tasks

tombuildsstuff added 2 commits November 27, 2016 08:00

Updating the linting

4ede5a5

Renaming the match the Composite spec

421093f

tombuildsstuff added 2 commits November 29, 2016 21:04

Fixing the build, probably ¯\_(ツ)_/¯

6dca3f5

Renaming the responses so as not to clash

f98c4ac

amarzavery reviewed Dec 6, 2016

View reviewed changes

d3r3kk mentioned this pull request Mar 3, 2017

[Do not merge] Addition of CRUD operations for Application Insights Components & Synthetic Monitors #988

Merged

10 tasks

salameer closed this Mar 17, 2017

tombuildsstuff mentioned this pull request Sep 25, 2019

Open Source the Wiki required for contributing to API Specifications #7315

Closed

Application Insights: CRUD for a new Instance #757

Application Insights: CRUD for a new Instance #757

Uh oh!

Conversation

tombuildsstuff commented Nov 23, 2016 • edited by mcardosos Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR information

Quality of Swagger

Uh oh!

azurecla commented Nov 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amarzavery commented Nov 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tombuildsstuff commented Nov 23, 2016

Uh oh!

tombuildsstuff commented Nov 23, 2016

Uh oh!

tombuildsstuff commented Nov 27, 2016

Uh oh!

tombuildsstuff commented Dec 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amarzavery commented Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tombuildsstuff commented Dec 7, 2016

Uh oh!

tombuildsstuff commented Dec 12, 2016

Uh oh!

salameer commented Dec 12, 2016

Uh oh!

SergeyKanzhelev commented Jan 4, 2017

Uh oh!

d3r3kk commented Jan 6, 2017

Uh oh!

d3r3kk commented Jan 26, 2017

Uh oh!

salameer commented Mar 17, 2017

Uh oh!

tombuildsstuff commented Mar 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tombuildsstuff commented Nov 23, 2016 •

edited by mcardosos

Loading

amarzavery commented Dec 6, 2016 •

edited

Loading