Skip to content

Renaming the Insights Swagger spec and generated package to Monitor#936

Merged
vishrutshah merged 9 commits intoAzure:masterfrom
AuxMon:Rename
Feb 16, 2017
Merged

Renaming the Insights Swagger spec and generated package to Monitor#936
vishrutshah merged 9 commits intoAzure:masterfrom
AuxMon:Rename

Conversation

@gucalder
Copy link
Copy Markdown
Contributor

@gucalder gucalder commented Feb 14, 2017

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.

@vishrutshah
Copy link
Copy Markdown
Contributor

Re-assigned it to me as I am already working with Guillermo & Laurent on this one.

Copy link
Copy Markdown
Contributor

@vishrutshah vishrutshah left a comment

Choose a reason for hiding this comment

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

@gucalder Overall changes looks good to me.

We have following questions for you:

  1. Only question I have is you have removed Channels from ActivityLogs & TenantActivityLogs in the monitor folder but you have not removed it from older insights folders. Is that intentional omission?
  2. Please fix errors from semantic & model validation because that is the first level of checks for correctness of the swagger that we are aiming for. That indicates something is wrong.

Semantic Validation: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/201673496

Model Validation: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/201673497

@gucalder
Copy link
Copy Markdown
Contributor Author

Yes, it is an intentional omission. The channels in Insights is working fine for .Net, but it is breaking Python due a serialization issue. It is a Flags Enum serialized as a string. It was specified as Enum in Swagger. .Net has no problem with that, but Python expects a single value from the Enum.

Other than the change in names and this omission above, the rest is just like it was in Insights.

@vishrutshah
Copy link
Copy Markdown
Contributor

@amarzavery / @veronicagg This change is only about renames but looks like many model validation and semantic validation issues already exists. Any comments/preferences how we should handle these things for now?

@gucalder
Copy link
Copy Markdown
Contributor Author

I made some changes. Here is some feedback:

  1. Null should be a valid value for a string. What I did there is to use an empty string in the example.

  2. The examples included some fields that are irrelevant for our spec, some are inserted by ARM, some are there for historic reasons, but they are not used by us at all. The validator does not allow that to happen. I just removed those fields from the examples. They could be considered as not required fields. They are ignored by the serialization/deserialization.

  3. In some cases the validator said there were errors in the example (additional fields), but there were not. The issue was that the object in the example is an instance of a polymorphic type. The instance in the example is correct, there should be no validation error in that case.

@gucalder
Copy link
Copy Markdown
Contributor Author

One check that is failing https://travis-ci.org/Azure/azure-rest-api-specs/jobs/201982907 due to the polymorphic type issue.

The others that are failing have nothing to do with us, except one that contains the same results from us as https://travis-ci.org/Azure/azure-rest-api-specs/jobs/201982907 plus results from some other folders in the repo.

@veronicagg
Copy link
Copy Markdown
Contributor

@vishrutshah we've been mostly deferring issues that are not introduced by the PR, so that it's not a lot of extra work on the person submitting the PR. Feel free to point them out and give the team the opportunity to fix them, if they don't get fixed, you can open a separate issue on those, or we'll be opening issues for all specs at once soon.

@vishrutshah
Copy link
Copy Markdown
Contributor

@lmazuel I've reviewed changes and seems good. Let me know if this looks okay to you, we can merge this in and we can have python sdk of azure-mgmt-monitor from that.

@gucalder
Copy link
Copy Markdown
Contributor Author

The last reference I put here was just to close #894 without merge since this PR covers it.

@vishrutshah vishrutshah merged commit efed632 into Azure:master Feb 16, 2017
@AutorestCI
Copy link
Copy Markdown

No modification for NodeJS

@AutorestCI
Copy link
Copy Markdown

@AutorestCI
Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants