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.

Sign up for GitHub

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

Jump to bottom

Add pagination and data plane support to PolicyInsights states and events APIs #15976

Merged
merged 4 commits into from
Oct 29, 2020
Merged

Add pagination and data plane support to PolicyInsights states and events APIs #15976

merged 4 commits into from
Oct 29, 2020

Conversation

NarineM
Copy link
Contributor

@NarineM NarineM commented Oct 14, 2020 •
edited
Loading

@NarineM NarineM mentioned this pull request Oct 14, 2020
Closed
@NarineM NarineM force-pushed the narinem-add-dataplane branch 2 times, most recently from df0783c to 9b1c2d1 Compare October 15, 2020 22:37
@NarineM NarineM force-pushed the narinem-add-dataplane branch from 9b1c2d1 to abe1fe3 Compare October 15, 2020 23:12
@NarineM
Copy link
Contributor Author

NarineM commented Oct 16, 2020

@allenjzhang , can you please review this change? I need to start on releasing this code as soon as possible.

markcowl
markcowl requested changes Oct 20, 2020
View reviewed changes
Copy link
Member

@markcowl markcowl 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 include a link to the swagger PR corresponding to this change.

...crosoft.Azure.Management.PolicyInsights/src/Microsoft.Azure.Management.PolicyInsights.csproj Outdated Show resolved Hide resolved
...yinsights/Microsoft.Azure.Management.PolicyInsights/src/Generated/IPolicyStatesOperations.cs
@@ -53,7 +53,7 @@ public partial interface IPolicyStatesOperations
/// <exception cref="Microsoft.Rest.ValidationException">
/// Thrown when a required parameter is null
/// </exception>
Task<AzureOperationResponse<PolicyStatesQueryResults>> ListQueryResultsForManagementGroupWithHttpMessagesAsync(string policyStatesResource, string managementGroupName, QueryOptions queryOptions = default(QueryOptions), Dictionary<string, List<string>> customHeaders = null, CancellationToken cancellationToken = default(CancellationToken));
Task<AzureOperationResponse<IPage<PolicyState>>> ListQueryResultsForManagementGroupWithHttpMessagesAsync(string policyStatesResource, string managementGroupName, QueryOptions queryOptions = default(QueryOptions), Dictionary<string, List<string>> customHeaders = null, CancellationToken cancellationToken = default(CancellationToken));
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change, correct? This is good, but we need to bump the major 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.

ok, i will change the major version too.

Copy link
Contributor Author

@NarineM NarineM Oct 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

No, it is not a breaking change from API point of view. but because there are quite some changes here, I agree that changing major version makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

It's a breaking change from the api point of view becauser the returned type has changed, and does not have the same properties and methods as the original type (IP{age versus PolicyStatesQueryResults

@markcowl markcowl assigned markcowl and unassigned allenjzhang Oct 20, 2020
markcowl
markcowl requested changes Oct 23, 2020
View reviewed changes
...crosoft.Azure.Management.PolicyInsights/src/Microsoft.Azure.Management.PolicyInsights.csproj Outdated
@@ -7,11 +7,13 @@
<PackageId>Microsoft.Azure.Management.PolicyInsights</PackageId>
<Description>Provides policy insights operations for Microsoft Azure.</Description>
<AssemblyName>Microsoft.Azure.Management.PolicyInsights</AssemblyName>
<Version>3.1.0</Version>
<Version>3.2.0</Version>
Copy link
Member

Choose a reason for hiding this comment

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

Version update also needed here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes, thanks for catching it. Updated.

@markcowl
Copy link
Member

/azp run net-mgmt -ci

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@markcowl
Copy link
Member

/azp run net - mgmt - ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@markcowl
Copy link
Member

/azp run net - core - ci

@azure-pipelines
Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@markcowl
Copy link
Member

@NarineM Sorry, should have been more explicit - you should change the version in the csproj as well as AssemblyInfo file. And, you will need to update the AssemblyVersion as well as the file version, due to the breaking change in the API surface

markcowl
markcowl requested changes Oct 23, 2020
View reviewed changes
sdk/policyinsights/Microsoft.Azure.Management.PolicyInsights/src/Properties/AssemblyInfo.cs Outdated
@@ -9,7 +9,7 @@
[assembly: AssemblyDescription("Provides Microsoft Azure policy insights operations.")]

[assembly: AssemblyVersion("3.0.0.0")]
Copy link
Member

Choose a reason for hiding this comment

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

Also, you really want to change AssemblyVersion - because the types are different, this could result in difficult to detect errors if this assembly were substituted for a previous version of this dll

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, Updated Assembly version as well.

@NarineM NarineM force-pushed the narinem-add-dataplane branch from efb3f6f to 6a67e72 Compare October 26, 2020 18:42
@NarineM NarineM force-pushed the narinem-add-dataplane branch from 6a67e72 to 86b5d5f Compare October 26, 2020 19:05
@NarineM
Copy link
Contributor Author

NarineM commented Oct 28, 2020

@markcowl Can you take at updated version and confirm if you are signing off? This PR is active for some time now, and I need to release a version out of this code base asap. Can you tell me how to expedite the review? thanks.

@markcowl markcowl merged commit 8f1c2ad into Azure:master Oct 29, 2020
annelo-msft pushed a commit to annelo-msft/azure-sdk-for-net that referenced this pull request Feb 17, 2021
@NarineM @annelo-msft
Add pagination and data plane support to PolicyInsights states and ev…
7a83b9a 
…ents APIs (Azure#15976)

* fix compile errors due to CP API changes

* update sdk package info and release notes

* fix merge validation errors

* bump up the major version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markcowl markcowl markcowl approved these changes

@allenjzhang allenjzhang Awaiting requested review from allenjzhang

@bquantump bquantump Awaiting requested review from bquantump

@m-nash m-nash Awaiting requested review from m-nash

@nisha-bhatia nisha-bhatia Awaiting requested review from nisha-bhatia

@YalinLi0312 YalinLi0312 Awaiting requested review from YalinLi0312

Assignees

@markcowl markcowl

@NarineM NarineM

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@NarineM @markcowl @allenjzhang