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.

Sign up for GitHub

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

Jump to bottom

Publish public preview swagger spec for "Azure metrics Ingestion REST API" #3856

Merged
merged 12 commits into from
Sep 14, 2018
Merged

Publish public preview swagger spec for "Azure metrics Ingestion REST API" #3856

merged 12 commits into from
Sep 14, 2018

Conversation

vickyv51
Copy link
Contributor

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

  • [Done] 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

@msftclas
Copy link

msftclas commented Sep 10, 2018 •
edited
Loading

CLA assistant check
All CLA requirements met.

@AutorestCI
Copy link

AutorestCI commented Sep 10, 2018 •
edited
Loading

Automation for azure-sdk-for-ruby

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

@AutorestCI
Copy link

AutorestCI commented Sep 10, 2018 •
edited
Loading

Automation for azure-sdk-for-python

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

@vickyv51 vickyv51 changed the title Publish "Azure metrics REST Ingestion API" public preview swagger spec Publish "Azure metrics Ingestion REST API" public preview swagger spec Sep 10, 2018
@vickyv51 vickyv51 changed the title Publish "Azure metrics Ingestion REST API" public preview swagger spec Publish public preview swagger spec for "Azure metrics Ingestion REST API" Sep 10, 2018
@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

jhendrixMSFT
jhendrixMSFT requested changes Sep 10, 2018
View reviewed changes
Copy link
Member

@jhendrixMSFT jhendrixMSFT left a comment

Choose a reason for hiding this comment

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

Missing README.md file, see here for an example.

@vickyv51
Address review comments
25acd4f 
 - Use a shorter operationId
 - Remove any non-success response code with default response clause
 - Add a new example and reference it in the swagger spec
 - Include readme.md file for the swagger spec
@vickyv51
Fix a bug in readme.md.
47b3f99 
 - Inline the readme.python.md file in the readme.md
jhendrixMSFT
jhendrixMSFT reviewed Sep 10, 2018
View reviewed changes
specification/monitor/data-plane/readme.md Outdated
output-folder: $(azure-libraries-for-java-folder)/azure-mgmt-monitor
```

# Validation
Copy link
Member

@jhendrixMSFT jhendrixMSFT Sep 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I'm guessing this Validation section is a result of copy/paste? If so you can remove it (lines 149+).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats correct. Dropping it.

@AutorestCI
Copy link

AutorestCI commented Sep 11, 2018 •
edited
Loading

Automation for azure-sdk-for-node

Nothing to generate for azure-sdk-for-node

@AutorestCI
Copy link

AutorestCI commented Sep 11, 2018 •
edited
Loading

Automation for azure-sdk-for-go

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

@AutorestCI
Copy link

AutorestCI commented Sep 11, 2018 •
edited
Loading

Automation for azure-sdk-for-java

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

jhendrixMSFT
jhendrixMSFT reviewed Sep 11, 2018
View reviewed changes
specification/monitor/data-plane/readme.md Outdated
output-folder: $(go-sdk-folder)/services/preview/monitor/mgmt/2018-03-01/insights
```

## Python
Copy link
Member

Choose a reason for hiding this comment

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

@lmazuel can you please review the config section for python?

jhendrixMSFT
jhendrixMSFT reviewed Sep 11, 2018
View reviewed changes
specification/monitor/data-plane/readme.md Outdated
output-folder: $(python-sdks-folder)/azure-mgmt-monitor
```

## Java
Copy link
Member

Choose a reason for hiding this comment

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

@jianghaolu can you please review the config section for Java?

jhendrixMSFT
jhendrixMSFT reviewed Sep 11, 2018
View reviewed changes
specification/monitor/data-plane/readme.md Outdated
``` yaml $(go)
go:
license-header: MICROSOFT_APACHE_NO_VERSION
namespace: insights
Copy link
Member

Choose a reason for hiding this comment

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

If the package directory is going to be monitor then this namespace will need to match it.

lmazuel
lmazuel reviewed Sep 11, 2018
View reviewed changes
specification/monitor/data-plane/readme.md Outdated
``` yaml $(python) && $(python-mode) == 'create'
python:
basic-setup-py: true
output-folder: $(python-sdks-folder)/azure-monitor/azure/dataplane/monitor
Copy link
Member

Choose a reason for hiding this comment

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

output-folder: $(python-sdks-folder)/azure-monitor/

@vickyv51
Address the PR comments
89a8ab4 
  - Fix the readme.md naming incompatibilities
  - Remove the c#-ish namespace prefix
This was referenced Sep 11, 2018
Merged
Merged
Merged
Merged
@jhendrixMSFT
Copy link
Member

Please take a look at the linter errors and model validation failures.

@vickyv51
-Remove assembly namespace prefix in few other places
e53b30d 
-Rename bunch of properties to follow camel case naming
-Update the enum types to have the "x-ms-enum" part of the json
jianghaolu
jianghaolu suggested changes Sep 12, 2018
View reviewed changes
specification/monitor/data-plane/readme.md Outdated Show resolved Hide resolved
specification/monitor/data-plane/readme.md Outdated Show resolved Hide resolved
@vickyv51
-Fix last batch of ARM Errors
d4de326 
-Update path and namespace for Java section
@jhendrixMSFT
Copy link
Member

There are multiple enum types named attributes that have different sets of values which is not allowed, see this build log. Either they all need to have the same set of values (which in that case just define it once and reference it) or their names need to be different.
Also you can run some of these validation tools locally, see https://github.com/Azure/oav for details.

@vickyv51
-Drop all the types that recursively came with ObjectResult and use s…
11666b8 
…imple object type instead. This should make the swagger spec clean and more easily maintainable in the long run
@jhendrixMSFT
Copy link
Member

Down to just two errors in the model validation regarding the example, see the log.

@jhendrixMSFT jhendrixMSFT merged commit ad44bbb into Azure:master Sep 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lmazuel lmazuel lmazuel left review comments

@jianghaolu jianghaolu jianghaolu approved these changes

@jhendrixMSFT jhendrixMSFT jhendrixMSFT approved these changes

@shahabhijeet shahabhijeet shahabhijeet approved these changes

Assignees

@jhendrixMSFT jhendrixMSFT

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@vickyv51 @msftclas @AutorestCI @azuresdkci @jhendrixMSFT @lmazuel @jianghaolu @shahabhijeet