Skip to content

Adding first version of Edge gateway sdk and tests#5250

Merged
dsgouda merged 7 commits intoAzure:masterfrom
anponnet:edgegatewaysdk
Feb 20, 2019
Merged

Adding first version of Edge gateway sdk and tests#5250
dsgouda merged 7 commits intoAzure:masterfrom
anponnet:edgegatewaysdk

Conversation

@anponnet
Copy link
Contributor

@anponnet anponnet commented Feb 14, 2019

Description


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.
  • The *.csproj and AssemblyInfo.cs files have been updated with the new version of the SDK.

@anponnet
Copy link
Contributor Author

@anponnet
Copy link
Contributor Author

@dsgouda
Copy link
Contributor

dsgouda commented Feb 14, 2019

@anponnet please confirm that you have joined the Azure org here

Copy link
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.

How was this code generated? Please create a generate.ps1 file similar to this and regenerate the code. Please commit all the changes generated.

<Import Condition=" Exists('$([MSBuild]::GetPathOfFileAbove(AzSdk.test.reference.props))') " Project="$([MSBuild]::GetPathOfFileAbove('AzSdk.test.reference.props'))" />
<!-- Please do not move/edit code below this line -->
<PropertyGroup>
<PackageId>Foo</PackageId>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rename this to EdgeGateway.Tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected using generate.ps1. Swagger was taken from the fork. https://github.com/anponnet/azure-rest-api-specs/tree/version20190301

<!-- Please do not move/edit code below this line -->
<PropertyGroup>
<PackageId>Foo</PackageId>
<VersionPrefix>1.0.0-preview</VersionPrefix>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please replace this with <Version>1.0.0</Version>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

<PropertyGroup>
<PackageId>Foo</PackageId>
<VersionPrefix>1.0.0-preview</VersionPrefix>
<AssemblyName>Foo</AssemblyName>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the AssemblyName

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

<PackageId>Foo</PackageId>
<VersionPrefix>1.0.0-preview</VersionPrefix>
<AssemblyName>Foo</AssemblyName>
<Description>BatchAI.Tests;</Description>
Copy link
Contributor

Choose a reason for hiding this comment

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

Incorrect name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

<PackageId>Foo</PackageId>
<VersionPrefix>1.0.0-preview</VersionPrefix>
<AssemblyName>Foo</AssemblyName>
<Description>BatchAI.Tests;</Description>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the line
<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
And fix errors if any

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

<PackageId>Microsoft.Azure.Management.EdgeGateway</PackageId>
<Description>Microsoft Azure Management EdgeGateway library</Description>
<AssemblyName>Microsoft.Azure.Management.EdgeGateway</AssemblyName>
<Version>1.0.0</Version>
Copy link
Contributor

Choose a reason for hiding this comment

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

We recommend starting versioning from 0.9.0-preview before releasing a stable version. Please confirm whether you are ready to release a stable version of the SDK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to 0.9.0-preview. May be we will release first version after 1-2 weeks

This is a public release of the Azure EdgeGateway SDK.
]]>
</PackageReleaseNotes>
<NoWarn>$(NoWarn);CS1591</NoWarn>
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why this was added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the line

Copy link
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.

Please add AssemblyInfo.cs files for Test and Sdk projects.

@anponnet
Copy link
Contributor Author

@dsgouda I am part of azure org

</PropertyGroup>

<PropertyGroup>
<TargetFrameworks>net452;netstandard1.4</TargetFrameworks>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please replace this with <TargetFrameworks>$(SdkTargetFx)</TargetFrameworks>


<PropertyGroup>
<TargetFrameworks>net452;netstandard1.4</TargetFrameworks>
<RootNamespace>Microsoft.Azure.Management.EdgeGateway</RootNamespace>
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think this line is needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


<!-- Please do not move/edit code below this line -->
<Import Condition=" Exists('$([MSBuild]::GetPathOfFileAbove(AzSdk.RP.props))') " Project="$([MSBuild]::GetPathOfFileAbove('AzSdk.RP.props'))" />
<ItemGroup>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this ItemGroup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

[assembly: AssemblyTitle("Microsoft Azure DataBox Edge Gateway Management Library")]
[assembly: AssemblyDescription("Provides management functionality for Microsoft Azure DataBox Edge Gateway Management Resources.")]

[assembly: AssemblyVersion("1.0.0.0")]
Copy link
Contributor

Choose a reason for hiding this comment

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

AssemblyVersion must also be 0.9.0.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -1 +1,2 @@
Start-AutoRestCodeGeneration -ResourceProvider "storSimple1200Series\resource-manager" -AutoRestVersion "latest" -SdkGenerationDirectory�"$PSScriptRoot\Generated"

Copy link
Contributor

Choose a reason for hiding this comment

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

Please undo changes to this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)|$(TargetFramework)|$(Platform)'=='Debug|netcoreapp2.0|AnyCPU'">
<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
<WarningsAsErrors />
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove lines 16 and 17

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

<ItemGroup>
<Service Include="{82a7f48d-3b50-4b1e-b82e-3ada8210c358}" />
</ItemGroup>
<ItemGroup>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use a regex like SessionRecords\**\*.json instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
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.

Looks great apart from a couple of minor corrections


[assembly: AssemblyVersion("1.0.0.0")]
[assembly: AssemblyVersion("0.9.0.0")]
[assembly: AssemblyFileVersion("1.0.0.0")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix AssemblyFileVersion too.

[assembly: AssemblyDescription("Provides management functionality for Microsoft Azure DataBox Edge Gateway Management Resources.")]

[assembly: AssemblyVersion("1.0.0.0")]
[assembly: AssemblyVersion("0.9.0.0")]
Copy link
Contributor

Choose a reason for hiding this comment

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

I probably misspoke earlier, we start versioning for new packages from 0.8.0-preview
Please update the csproj and this file to set all versions to 0.8.0 (-preview in the csproj)

Copy link
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.

Also, please rename the directory Microsoft.Azure.Management.EdgeGateway to Management.EdgeGateway for consistency

Copy link
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.

Approving in principle, will merge once Rest spec is merged and code is regenerated

Copy link
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.

Please remove the $(SdkTargetFx) file

@anponnet
Copy link
Contributor Author

Please remove the $(SdkTargetFx) file

@dsgouda I did not understand the comment. Should I remove generate.ps1 file?

@anponnet
Copy link
Contributor Author

@dsgouda : Rest spec is merged and code is regenerated

@dsgouda
Copy link
Contributor

dsgouda commented Feb 19, 2019

@anponnet can you check if you have joined the Azure org here

@dsgouda
Copy link
Contributor

dsgouda commented Feb 19, 2019

@azuresdkci Test this please

@dsgouda dsgouda changed the base branch from psSdkJson6 to master February 19, 2019 18:51
@dsgouda
Copy link
Contributor

dsgouda commented Feb 19, 2019

@anponnet please note that we have switched our base branch to master. That was the problem here.

Copy link
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.

Please delete the file $(SdkTargetFx)

<PropertyGroup>
<TargetFrameworks>netcoreapp2.0</TargetFrameworks>
</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)|$(TargetFramework)|$(Platform)'=='Debug|netcoreapp2.0|AnyCPU'">
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove these conditions

<!-- Please do not move/edit code below this line -->
<PropertyGroup>
<PackageId>EdgeGateway.Tests</PackageId>
<VersionPrefix>1.0.0</VersionPrefix>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please replace VersionPrefix with Version

<PackageTags>Microsoft Azure EdgeGateway management;EdgeGateway;EdgeGateway management;</PackageTags>
<PackageReleaseNotes>
<![CDATA[
This is a public release of the Azure EdgeGateway SDK.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "public preview release"

Copy link
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 will merge once CIs pass

@dsgouda dsgouda merged commit bf60664 into Azure:master Feb 20, 2019
mentat9 pushed a commit to mentat9/azure-sdk-for-net that referenced this pull request Jun 10, 2019
* Adding first version of Edge gateway sdk and tests

* Fixing PR comments - Project property corrections, add assemblyinfo, use generate.ps1

* Fix PR comments - assemblyinfo, assemblyversion, unnecassary properties in proj files

* PR comment fixes: Target framework, folder structure

* Updated device updates test case

* Regenerated client code with generate.ps1 pointing to azure master rest spec

* PR Comments: project property fixes, rerun tests
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.

2 participants