Skip to content

Add IotCentral ARM SDK#4462

Merged
dsgouda merged 1 commit intoAzure:psSdkJson6from
emgarten:emgarten/iotCentralSDK
Jun 19, 2018
Merged

Add IotCentral ARM SDK#4462
dsgouda merged 1 commit intoAzure:psSdkJson6from
emgarten:emgarten/iotCentralSDK

Conversation

@emgarten
Copy link
Copy Markdown
Member

@emgarten emgarten commented Jun 19, 2018

Description

  • Add C# SDK for Iot Central for the first time
  • Add Iot Central SDK tests

API Spec PR: Azure/azure-rest-api-specs#3185

This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

  • Title of the pull request 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 more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

SDK Generation Guidelines

  • If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
  • The generate.cmd file for the SDK has been updated with the version of AutoRest, as well as the commitid of your swagger spec or link to the swagger spec, used to generate the code. (@emgarten: Uses latest, let me know if this should be changed. I based the generate files off other SDKs in the repo)
  • The *.csproj and AssemblyInfo.cs files have been updated with the new version of the SDK. (@emgarten: this is set in the csproj)

dsgouda
dsgouda suggested changes Jun 19, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@dsgouda dsgouda left a comment

Choose a reason for hiding this comment

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

LGTM for the most part, left a few comments.

src/SDKs/IotCentral/Management.IotCentral/generate.ps1 Outdated
Copy link
Copy Markdown
Contributor

@dsgouda dsgouda Jun 19, 2018

Choose a reason for hiding this comment

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

generate.ps1 uses powershell cmdlet now, please take a look at this and make appropriate changes and regenerate the code.

Copy link
Copy Markdown
Member Author

@emgarten emgarten Jun 19, 2018

Choose a reason for hiding this comment

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

I've updated the ps1 script and generated the files again to make sure everything is in sync.

src/SDKs/IotCentral/Management.IotCentral/Microsoft.Azure.Management.IotCentral.csproj Outdated
Copy link
Copy Markdown
Contributor

@dsgouda dsgouda Jun 19, 2018

Choose a reason for hiding this comment

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

nit: indentation is usually 2 spaces.

Copy link
Copy Markdown
Member Author

@emgarten emgarten Jun 19, 2018

Choose a reason for hiding this comment

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

Fixed, there was a mix of tabs and spaces here.

src/SDKs/IotCentral/Management.IotCentral/generate.cmd Outdated
Copy link
Copy Markdown
Contributor

@dsgouda dsgouda Jun 19, 2018

Choose a reason for hiding this comment

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

generate.cmds are not needed anymore, all code generation must go through generate.ps1s

Copy link
Copy Markdown
Member Author

@emgarten emgarten Jun 19, 2018

Choose a reason for hiding this comment

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

removed

@emgarten
Copy link
Copy Markdown
Member Author

emgarten commented Jun 19, 2018

Thanks for the feedback @dsgouda I've updated the PR

@emgarten
Copy link
Copy Markdown
Member Author

emgarten commented Jun 19, 2018

//cc @vishwam

dsgouda
dsgouda suggested changes Jun 19, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@dsgouda dsgouda left a comment

Choose a reason for hiding this comment

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

Typo resulted in incorrect log file being generated.

src/SDKs/IotCentral/Management.IotCentral/generate.ps1 Outdated
Copy link
Copy Markdown
Contributor

@dsgouda dsgouda Jun 19, 2018

Choose a reason for hiding this comment

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

Please fix the ResourceProvider here, should be iotcentral/resource-manager

Copy link
Copy Markdown
Member Author

@emgarten emgarten Jun 19, 2018

Choose a reason for hiding this comment

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

Good catch, I've updated this and regenerated everything

src/SDKs/_metadata/iotcentral_resource-manage.txt Outdated
Copy link
Copy Markdown
Contributor

@dsgouda dsgouda Jun 19, 2018

Choose a reason for hiding this comment

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

Please delete this file and regenerate code

Copy link
Copy Markdown
Member Author

@emgarten emgarten Jun 19, 2018

Choose a reason for hiding this comment

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

I've cleaned this up

dsgouda
dsgouda suggested changes Jun 19, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@dsgouda dsgouda left a comment

Choose a reason for hiding this comment

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

Almost there, please address the remaining comments.

src/SDKs/IotCentral/Management.IotCentral/Microsoft.Azure.Management.IotCentral.csproj Outdated
Copy link
Copy Markdown
Contributor

@dsgouda dsgouda Jun 19, 2018

Choose a reason for hiding this comment

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

If you are planning to release a preview version, the versioning must start from 0.9.0-preview

Copy link
Copy Markdown
Contributor

@dsgouda dsgouda Jun 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I missed this earlier but we also need Assembly.cs files similar to this for both the projects with the assembly versions properly set.

Copy link
Copy Markdown
Member Author

@emgarten emgarten Jun 19, 2018

Choose a reason for hiding this comment

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

I've added Assembly.cs and changed the csproj version to 0.9.0-preview

image

@emgarten
Copy link
Copy Markdown
Member Author

emgarten commented Jun 19, 2018

@dsgouda is anything else needed for this?

dsgouda
dsgouda approved these changes Jun 19, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@dsgouda dsgouda left a comment

Choose a reason for hiding this comment

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

LGTM

@dsgouda dsgouda merged commit 7839ba8 into Azure:psSdkJson6 Jun 19, 2018
@emgarten
Copy link
Copy Markdown
Member Author

emgarten commented Jun 25, 2018

@dsgouda when will the package for this be on nuget.org?

@dsgouda
Copy link
Copy Markdown
Contributor

dsgouda commented Jun 25, 2018

@emgarten we don't publish the packages. Please read the instructions to publish the package here.

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

Reviewers

1 more reviewer

@dsgouda dsgouda dsgouda approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@emgarten @dsgouda