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.

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

SDK changes for traffic analytics parameters added to existing networkwatcher flowlog parameters #4320

Closed
wants to merge 8 commits into from

Conversation

sayghosh
Copy link
Contributor

@sayghosh sayghosh commented May 15, 2018

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.

@sayghosh
Copy link
Contributor Author

Here is the REST spec PR link: Azure/azure-rest-api-specs#3023

@dsgouda
Copy link
Contributor

dsgouda commented May 15, 2018

@MikhailTryakhov is the official may release for network sdk repo? Please confirm

@MikhailTryakhov
Copy link
Contributor

MikhailTryakhov commented May 15, 2018

@dsgouda no it's not. The deadline of swagger is May, 17
@sayghosh you have to use minot version change 19.0.0.0->19.0.0.1 or something like that.
The official 20.0.0.0 one would be able later.

@sayghosh
Copy link
Contributor Author

@MikhailTryakhov pushed it down to 19.0.1.0.

@MikhailTryakhov
Copy link
Contributor

Thanks, LGTM
@dsgouda if you're still not sure we can sync up with Samer - this is my first experience with minor versions publishing :)
Let's help Sayantan with publishing this version soon - he worked really hard to do everything in time ;)

@sayghosh
Copy link
Contributor Author

sayghosh commented May 16, 2018

Build failure cause:
image
Is this an intermittent issue? If so, can we re trigger the build? For reference, here is a successful build for the Network module on the same fork with latest changes: https://azuresdkci.westus2.cloudapp.azure.com/job/NetCore-SDK-Sign/327/

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.

Couple of minor changes apart looks good

@@ -7,7 +7,7 @@
<PackageId>Microsoft.Azure.Management.Network</PackageId>
<Description>Provides management capabilities for Network services.</Description>
<AssemblyName>Microsoft.Azure.Management.Network</AssemblyName>
<Version>19.0.0-preview</Version>
<Version>19.0.1-preview</Version>
<PackageTags>Microsoft Azure Network management;Network;Network management;</PackageTags>
<PackageReleaseNotes>
<![CDATA[
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Since there are breaking changes, the minor version must be updated, i.e., 19.1.0-preview
  2. PackageReleaseNotes must be updated

@dsgouda
Copy link
Contributor

dsgouda commented May 16, 2018

@sayghosh looks like the signing build was successful

@sayghosh
Copy link
Contributor Author

@dsgouda Yes it succeeded in second attempt. First time it failed due to some intermittent issue. And addressed the changes requested.

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 subject to CIs passing


1) azure-rest-api-specs repository information
GitHub user: Azure
Branch: master
Commit: b9721ae4c9f04914cc89260b8339dc33372f835b
Branch: Network-2018-05-01
Copy link
Contributor

Choose a reason for hiding this comment

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

@sayghosh We cannot accept this PR. The SDK MUST be generated from master branch of REST specs repo.
Please coordinate with @MikhailTryakhov to create a PR for the May branch and integrate your changes there. `

@dsgouda dsgouda closed this May 16, 2018
@MikhailTryakhov
Copy link
Contributor

Hi @sayghosh
the major version would be ready for review tomorrow including all your updates. Can you please wait for it?

@sayghosh
Copy link
Contributor Author

@MikhailTryakhov Sure :)

@sayghosh
Copy link
Contributor Author

@dsgouda Please add this requirement as part of the "SDK Generation Guidelines" above so that no duplicate effort goes in raising a wrong PR from any other branch :)

@dsgouda
Copy link
Contributor

dsgouda commented May 17, 2018

Thanks @sayghosh we will update the documentation

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

Successfully merging this pull request may close these issues.

3 participants