Skip to content

Conversation

@sojain
Copy link

@sojain sojain commented Sep 12, 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

Currently raised against preview, once signed off will move it to stable

@AutorestCI
Copy link

AutorestCI commented Sep 12, 2018

Automation for azure-sdk-for-python

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

@AutorestCI
Copy link

AutorestCI commented Sep 12, 2018

Automation for azure-sdk-for-ruby

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

@AutorestCI
Copy link

AutorestCI commented Sep 12, 2018

Automation for azure-sdk-for-java

Encountered a Subprocess error: (azure-sdk-for-java)

Command: bundle install && rake arm:regen_all_profiles['azure_mgmt_databox']
Finished with return code 10
and output:

Don't run Bundler as root. Bundler can ask for sudo if it is needed, and
installing your bundle as root will break this application for all non-root
users on this machine.
Could not locate Gemfile

@AutorestCI
Copy link

AutorestCI commented Sep 12, 2018

Automation for azure-sdk-for-node

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

@AutorestCI
Copy link

AutorestCI commented Sep 12, 2018

Automation for azure-sdk-for-go

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

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@anuchandy anuchandy added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Sep 12, 2018
Copy link
Member

@anuchandy anuchandy left a comment

Choose a reason for hiding this comment

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

in general modelAsString:false has extensibility constrain, i.e. if service introduce a new value for the enum in the same api-version then SDKs can break. If we are not anticipating any new values in the current api-version then we are good here otherwise keep modelAsString:false. (for the discriminator specific enums you can keep modelAsString:false).

@anuchandy
Copy link
Member

anuchandy commented Sep 12, 2018

@ravbhatnagar please take a look, this is preview feature additions. Also need your input on linked access refer this.

@anuchandy
Copy link
Member

@sojain between you will need to update your readme file to have tags, like below:

DataBox

see https://aka.ms/autorest

This is the AutoRest configuration file for DataBox.


Getting Started

To build the SDK for DataBox, simply Install AutoRest and in this folder, run:

autorest

To see additional help and options, run:

autorest --help


Configuration

Basic Information

These are the global settings for the DataBox API.

openapi-type: arm
tag: package-2018-01

Tag: package-2018-01

These settings apply only when --tag=package-2018-01 is specified on the command line.

input-file:
- Microsoft.DataBox\preview\2018-01-01\databox.json

Suppression

directive:
  - suppress:
    - R2016 #to suppress (PatchBodyParametersSchema/R2016/RPCViolation)
    - R2062 #to suppress (XmsResourceInPutResponse/R2062/RPCViolation)

Code Generation

Swagger to SDK

This section describes what SDK should be generated by the automatic system.
This is not used by Autorest itself.

swagger-to-sdk:
  - repo: azure-sdk-for-go
  - repo: azure-sdk-for-node

C#

These settings apply only when --csharp is specified on the command line.
Please also specify --csharp-sdks-folder=<path to "SDKs" directory of your azure-sdk-for-net clone>.

csharp:
  azure-arm: true
  license-header: MICROSOFT_MIT_NO_VERSION
  namespace: Microsoft.Azure.Management.DataBox
  output-folder: $(csharp-sdks-folder)/DataBox/Management.DataBox/Generated
  clear-output-folder: true

Go

These settings apply only when --go is specified on the command line.

go:
  license-header: MICROSOFT_APACHE_NO_VERSION
  namespace: databox
  clear-output-folder: true

Go multi-api

batch:
  - tag: package-2018-01

Tag: package-2018-01 and go

Please also specify --go-sdk-folder=<path to the root directory of your azure-sdk-for-go clone>.

output-folder: $(go-sdk-folder)/services/preview/databox/mgmt/2018-01-01/databox

Java

These settings apply only when --java is specified on the command line.
Please also specify --azure-libraries-for-java-folder=<path to the root directory of your azure-libraries-for-java clone>.

java:
  azure-arm: true
  fluent: true
  namespace: com.microsoft.azure.management.databox
  license-header: MICROSOFT_MIT_NO_CODEGEN
  payload-flattening-threshold: 1
  output-folder: $(azure-libraries-for-java-folder)/databox

Java multi-api

batch:
  - tag: package-2018-01

Tag: package-2018-05 and java

These settings apply only when --tag=package-2018-01 --java is specified on the command line.
Please also specify --azure-libraries-for-java-folder=<path to the root directory of your azure-sdk-for-java clone>.

java:
  namespace: com.microsoft.azure.management.databox.v2018_01_01
  output-folder: $(azure-libraries-for-java-folder)/databox/resource-manager/v2018_01_01
regenerate-manager: true
generate-interface: true

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.

Even though this is a preview version, these changes need to correspond to a new API version to avoid breaking customers. Some of these deleted POST apis are actively being used, and a deprecation plan is advised.

1) Removing unnecessary fields
2) Correctly populating modelasstring as modelasextensible
3) adding descriptions for enum
@sojain
Copy link
Author

sojain commented Sep 14, 2018

@anuchandy , i have taken most of the comments and also the readme update, however post that build is not working, can you help me in finding the root cause

Looks like there was a space issue, fixed it

2) Moving to stable folder
@anuchandy
Copy link
Member

@sojain I've created readme file for databox here https://raw.githubusercontent.com/anuchandy/sample-readme/master/readme.md which includes settings for all languages. Can you try this?

@anuchandy
Copy link
Member

@sojain you also need sdks for all languages right? the readme contains entries for C# and Go only. The link i shared contains entries for all languages.

@sojain
Copy link
Author

sojain commented Sep 17, 2018

@anuchandy , thanks for sharing this.

This failed with
Command: bundle install && rake arm:regen_all_profiles['azure_mgmt_databox']
Finished with return code 10
and output:
Don't run Bundler as root. Bundler can ask for sudo if it is needed, and
installing your bundle as root will break this application for all non-root
users on this machine.
Could not locate Gemfile

It works if I remove bundle command, any suggestions. Also just fo rmy knowledge did you some tool to generate the readme.md

@anuchandy
Copy link
Member

@lmazuel the readme's swagger-to-sdk section looks like below:

swagger-to-sdk:
  - repo: azure-sdk-for-python
  - repo: azure-sdk-for-node
  - repo: azure-sdk-for-go
  - repo: azure-sdk-for-ruby
  - repo: azure-sdk-for-java
    after_scripts:
      - bundle install && rake arm:regen_all_profiles['azure_mgmt_databox']

The command bundle install && rake arm:regen_all_profiles['azure_mgmt_databox'] to generate the ruby sdk is failing with error

Command: bundle install && rake arm:regen_all_profiles['azure_mgmt_databox']
Finished with return code 10
and output:
Don't run Bundler as root. Bundler can ask for sudo if it is needed, and
installing your bundle as root will break this application for all non-root
users on this machine.
Could not locate Gemfile

Talked to @sarangan12 about this, he mentioned that this command shouldn't be run as a root user. Can you take a look and help here?

@anuchandy
Copy link
Member

@sojain i manually author the readme, it's not generated.

@sarangan12
Copy link
Contributor

@anuchandy The error is expected. It is not because of the root user. The azure_mgmt_databox is a new gem which does not have a skeleton structure. So, it is failing. You can ignore this error

@KrisBash KrisBash added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed ARMReviewInProgress WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Sep 17, 2018
@sojain
Copy link
Author

sojain commented Sep 18, 2018

thanks @anuchandy for all the help, who can help me in closing this pull request given I have arm signoff

@anuchandy
Copy link
Member

@sojain - is this api-version deployed to at least one production region? if yes, we're good to merge. let me know.

@sojain
Copy link
Author

sojain commented Sep 18, 2018

@anuchandy yes api version is deployed to all regions

@sojain
Copy link
Author

sojain commented Sep 19, 2018

@anuchandy, sorry to bother, but can you please help in completing this pull request, we have GA at ignite on 24th and hence the urgency

@anuchandy anuchandy merged commit be799c3 into Azure:master Sep 19, 2018
@AutorestCI
Copy link

AutorestCI commented Sep 19, 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

ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants