Skip to content

Conversation

@bganapa
Copy link
Member

@bganapa bganapa commented Aug 1, 2018

Description

Checklist

Copy link
Member

Choose a reason for hiding this comment

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

Public API surface cannot be changed in this way - you could add a new method, but not change the signature of an existing one

Copy link
Member Author

Choose a reason for hiding this comment

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

sure.. I will do that.. thought abt doing it, wanted to get your opinion before doing the extra work :)

Copy link
Member

Choose a reason for hiding this comment

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

Same - this is a disallowed breaking change

Copy link
Member

Choose a reason for hiding this comment

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

Same. We could add a new property which was a string, but we cannot change the siagnature of an existing property

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.

Additionally, once this is fixed, can we add a recorded test from AzureStack, to prevent regressions in the future.

@maddieclayton
Copy link
Contributor

@bganapa You will need to move the changes to src/Common here: https://github.com/Azure/azure-powershell-common. Once you create a PR there, run this job with your fork and branch: https://azuresdkci.westus2.cloudapp.azure.com/view/PowerShell/job/ps-common-sign/. Take the relevant packages, copy them into local feed, and from there you can test the common changes. Let me know if you have any questions about this process.

@bganapa
Copy link
Member Author

bganapa commented Aug 26, 2018

closing this PR infavor of Azure/azure-powershell-common#30

@bganapa bganapa closed this Aug 26, 2018
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.

4 participants