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 swagger update to support AutomaticRepairsPolicy for Virtual Machine Scale Sets #8141

Merged
merged 5 commits into from
Oct 22, 2019

Conversation

hapandar
Copy link
Contributor

@hapandar hapandar commented Oct 16, 2019

This SDK PR includes the below Swagger updates:

Azure/azure-rest-api-specs#7484

Azure/azure-rest-api-specs#7513

Azure/azure-rest-api-specs#7355

Azure/azure-rest-api-specs#7034

Resolved. Including the additional changes
Note: when I ran code gen tool, there were other changes (which were not part of my swagger changes) that were generated as well. Is the best practice to revert these changes?

  1. Since I am not familiar with the changes that were additionally generated and there may not be test coverage for this
  2. There could already be other PRs in progress that would generate these additional properties along with tests.
    Please let me know and I can plan to revert those changes manually. For example, the ScaleInPolicy is not part of my changes and is not covered as part of the tests I have written.

@hapandar hapandar requested a review from erich-wang as a code owner October 16, 2019 17:04
[assembly: AssemblyVersion("29.1.0.0")]
[assembly: AssemblyFileVersion("29.0.0.0")]
[assembly: AssemblyVersion("29.2.0.0")]
[assembly: AssemblyFileVersion("29.2.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.

You don't have to update AssemblyFileVersion for minor version update. Leave it to 29.0.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.

Copy link
Member

@isra-fel isra-fel Oct 18, 2019

Choose a reason for hiding this comment

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

@hyonholee @hapandar That is not correct. AssemblyFileVersion needs to be updated for every release, AssemblyVersion only when introducing a breaking change (aka a major version update).
So my suggestion is:

[assembly: AssemblyVersion("29.1.0.0")]
[assembly: AssemblyFileVersion("29.2.0.0")]

Read about it in our process document: https://github.com/Azure/adx-documentation-pr/blob/master/engineering/adx_netsdk_process.md#signing-and-publishing-nuget-packges
An article explaining this: https://support.microsoft.com/en-us/help/556041

Copy link
Contributor

Choose a reason for hiding this comment

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

@hapandar Sorry, I was confused. @isra-fel is right. Change only AssemblyFileVersion.

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

@hyonholee hyonholee assigned hyonholee and erich-wang and unassigned hyonholee Oct 17, 2019
@hyonholee
Copy link
Contributor

LGTM

Copy link
Member

@isra-fel isra-fel left a comment

Choose a reason for hiding this comment

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

Please see my comment in the above conversation. Thanks

@isra-fel isra-fel assigned hapandar and unassigned isra-fel Oct 18, 2019
@isra-fel isra-fel added needs-revision Mgmt This issue is related to a management-plane library. and removed needs-review labels Oct 18, 2019
@isra-fel
Copy link
Member

/azp run net - mgmt - ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@isra-fel isra-fel merged commit 3272a17 into Azure:master Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mgmt This issue is related to a management-plane library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants