[App Insights] Sync to swagger, bump to 1.0.0-preview-1, rename package *breaking changes* by alexeldeib · Pull Request #4790 · Azure/azure-sdk-for-net

alexeldeib · 2018-09-24T21:42:07Z

Description

Azure/azure-rest-api-specs#3771
Azure/azure-rest-api-specs#3084 --> big one
Azure/azure-rest-api-specs#3385
Azure/azure-rest-api-specs#3997

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

Please add REST spec PR link to the SDK PR
I have read the contribution guidelines.
The pull request does not introduce breaking changes.

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.

…Preview-1

alexeldeib · 2018-09-24T22:08:23Z

I've been refining the data plane swagger for Application Insights and working towards 1.0.0 release for all of our SDKs. This PR rolls up a few changes I've made:

break Metrics, Events, and Queries into separate operation groups
minor operation renaming
move appId parameter from client to method (quality of life; makes sense to new up one client and query with different parameters)
various type fixes: some timespans need to be strings, some enums should be refined/loosened.

Adding on to that, this is an SDK to query data plane information -- metrics, logs and events for Azure resources. Application Insights also has numerous clients for sending telemetry to Azure, e.g. Microsoft.ApplicationInsights.
After discussing this with App Insights folks and Azure SDK PMs, I'd like to propose renaming this package Microsoft.Azure.ApplicationInsights.Query, to clarify any ambiguity between the two. I've already discussed and begun implementing similar changes with Python, Java, and Node SDKs.

I've made the relevant code changes already, but I wanted to ask how to go about this procedurally? Is there a precedent for package rename? Could we e.g., leave a note on the Microsoft.Azure.ApplicationInsights package page with a deprecation notice pointing to the newly named package? Would love to get thoughts on this.

Thanks!

alexeldeib · 2018-09-24T22:43:11Z

Incomplete

Update comments for documentation to reflect AppID parameter
Changelog

dsgouda · 2018-09-24T22:31:48Z

...ationInsights/DataPlane/ApplicationInsights/Microsoft.Azure.ApplicationInsights.Query.csproj

-    <AssemblyName>Microsoft.Azure.ApplicationInsights</AssemblyName>
-    <PackageId>Microsoft.Azure.ApplicationInsights</PackageId>
-    <PackageTags>Management.ApplicationInsights;</PackageTags>
+    <VersionPrefix>1.0.0.0</VersionPrefix>


versioning should be 1.0.0
Please rename VersionPrefix with Version

dsgouda · 2018-09-24T22:32:56Z

...ationInsights/DataPlane/ApplicationInsights/Microsoft.Azure.ApplicationInsights.Query.csproj

    <TargetFrameworks>net452;netstandard1.4</TargetFrameworks>
-    <RootNamespace>Microsoft.Azure.ApplicationInsights</RootNamespace>
+    <RootNamespace>Microsoft.Azure.ApplicationInsights.Query</RootNamespace>
+    <Version>1.0.0-Preview-1</Version>


Please remove this

dsgouda · 2018-09-24T22:33:29Z

...ationInsights/DataPlane/ApplicationInsights/Microsoft.Azure.ApplicationInsights.Query.csproj

-    <PackageId>Microsoft.Azure.ApplicationInsights</PackageId>
-    <PackageTags>Management.ApplicationInsights;</PackageTags>
+    <VersionPrefix>1.0.0.0</VersionPrefix>
+    <VersionSuffix>Preview-1</VersionSuffix>


Please remove this
Also, is the package stable or preview?
If this is still preview, the version can be bumped to 0.10.0-preview instead

dsgouda · 2018-09-24T22:34:37Z

src/SDKs/_metadata/ApplicationInsights_data-plane.txt

-GitHub user: Azure
-Branch:      master
-Commit:      d122af3086f0b1693b9c506befc497a1f4a8b8e1
+GitHub user: alexeldeib


Is the spec merged?

There's an open PR which changes an enum to modelAsString. Rest of the changes are merged.

I can regenerate after it's merged. The same PR contains the namespace change.

dsgouda · 2018-09-24T22:52:40Z

@alexeldeib As such you cannot rename a package, we can hide/deprecate the old package with a message that redirects to a new package and then publish whichever new packages are needed. Please start an email thread if you need further details

alexeldeib · 2018-09-25T00:55:13Z

@dsgouda confirming, for preview version you want 0.10.0-preview not 1.0.0-preview? I made it 0.10.0-preview currently.

dsgouda · 2018-09-25T15:24:14Z

@alexeldeib there should be a strong reason for major version bumps (if the REST spec version is new or if breaking changes are introduced to a stable version)
This looks like a preview version without revisions to the REST spec version itself, so 0.10.0-preview is the right version.
Having said that, let's put a pin in this until we finalize upon a plan for renaming the namespace and package name

…t into syncInsights

dsgouda

Looks good apart from the one comment

dsgouda · 2018-09-28T22:30:30Z

...ationInsights/DataPlane/Data.ApplicationInsights.Tests/Data.ApplicationInsights.Tests.csproj

@@ -14,7 +14,9 @@
  </ItemGroup>
  <ItemGroup>


Please replace VersionPrefix with Version
Also add <TreatWarningsAsErrors>true</TreatWarningsAsErrors> to the same PropertyGroup

dsgouda

LGTM subject to CIs passing

alexeldeib · 2018-10-02T23:36:03Z

@dsgouda Bump since CI is green?

Ace Eldeib added 2 commits September 22, 2018 20:41

Revisions for 1.0.0-preview

1daa4b8

Rename to Microsoft.Azure.ApplicationInsights.Query, bump to 1.0.0.0-…

d444b09

…Preview-1

alexeldeib changed the title ~~[App Insights] Sync to swagger, bump to 1.0.0-preview-1~~ [App Insights] Sync to swagger, bump to 1.0.0-preview-1, rename package *breaking changes* Sep 24, 2018

dsgouda added Changes Requested labels Sep 24, 2018

dsgouda suggested changes Sep 24, 2018

View reviewed changes

alexeldeib mentioned this pull request Sep 24, 2018

Recommend path for pulling Application Insights data #4785

Closed

version fix

a5eec25

Ace Eldeib and others added 4 commits September 25, 2018 15:15

revert version for new package

a8b33e7

Merge branch 'psSdkJson6' into syncInsights

b1b7695

regenerate from master branch

6c082dd

Merge branch 'syncInsights' of github.com:alexeldeib/azure-sdk-for-ne…

e1823e4

…t into syncInsights

dsgouda suggested changes Sep 28, 2018

View reviewed changes

dsgouda removed the spec-not-merged label Sep 28, 2018

Fix version, treatWarningAsError, fix warnings, add docstrings

0a2320c

dsgouda approved these changes Sep 28, 2018

View reviewed changes

dsgouda removed the Changes Requested label Oct 2, 2018

dsgouda merged commit 274e7d0 into Azure:psSdkJson6 Oct 2, 2018

Conversation

alexeldeib commented Sep 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Azure/azure-rest-api-specs#3771 Azure/azure-rest-api-specs#3084 --> big one Azure/azure-rest-api-specs#3385 Azure/azure-rest-api-specs#3997

Uh oh!

alexeldeib commented Sep 24, 2018

Uh oh!

alexeldeib commented Sep 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Incomplete

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsgouda commented Sep 24, 2018

Uh oh!

alexeldeib commented Sep 25, 2018

Uh oh!

dsgouda commented Sep 25, 2018

Uh oh!

dsgouda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsgouda left a comment

Choose a reason for hiding this comment

Uh oh!

alexeldeib commented Oct 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alexeldeib commented Sep 24, 2018 •

edited

Loading

Azure/azure-rest-api-specs#3771
Azure/azure-rest-api-specs#3084 --> big one
Azure/azure-rest-api-specs#3385
Azure/azure-rest-api-specs#3997

alexeldeib commented Sep 24, 2018 •

edited

Loading