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

Restructure Changelog Logic #823

Merged
merged 2 commits into from
Sep 4, 2020

Conversation

chidozieononiwu
Copy link
Member

Restructure changelog logic to remove reliance on modules and make use of eng\common\scripts\common.ps1

@chidozieononiwu chidozieononiwu requested a review from a team as a code owner July 28, 2020 19:09
@chidozieononiwu chidozieononiwu force-pushed the RestructureChangeLog branch 2 times, most recently from e9293e0 to 34a1253 Compare July 28, 2020 19:42
@chidozieononiwu chidozieononiwu force-pushed the RestructureChangeLog branch 3 times, most recently from 3e87c44 to 0457e4b Compare July 29, 2020 03:34
@chidozieononiwu chidozieononiwu force-pushed the RestructureChangeLog branch 2 times, most recently from 5437296 to a357775 Compare July 30, 2020 18:04
@@ -3,17 +3,11 @@ param (
[String]$ChangeLogLocation,
[String]$VersionString,
[string]$PackageName,
[string]$ServiceName,
Copy link
Member

Choose a reason for hiding this comment

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

These changes will break any consumers so somehow we need to figure out a way of not breaking them.

One potential way is to make changes additive and then go back in after you moved everyone to the new approach and remove the unused parameters.

Copy link
Member

Choose a reason for hiding this comment

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

Of course this is only an issue if there is anyone directly calling this script. If they are all using the yml template you can do what I suggested in my comment in the template to handle the break.

Copy link
Member Author

Choose a reason for hiding this comment

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

I decided to go with making the changes additive. So I added the new files and will switch then over in the languages repo so I can make sure it all works and there is no tome where things are broken. The only down side is that I have appended -2 to the files names which is a little weird.

Copy link
Member

Choose a reason for hiding this comment

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

I personally wouldn't create a new template but instead just make the changes to the existing template additive like I suggested. If you add the second template you will have the same problem when you want to clean that script up.

@@ -2,7 +2,7 @@ parameters:
- name: PackageName
type: string
default: 'not-specified'
- name: ServiceName
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps as a transition you have both ServiceName and ServiceDirectory and you do something like coalesce(parameters.ServiceDirectory, parameters.ServiceName), and then once you have converted all the usages you can remove the ServiceName parameter.

Copy link
Member

@weshaggard weshaggard left a comment

Choose a reason for hiding this comment

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

Lets correctly order this tools change and the language changes to ensure we don't have any breaks. Even if that means duplicate parameters temporarily.

@chidozieononiwu chidozieononiwu force-pushed the RestructureChangeLog branch 2 times, most recently from f8589d8 to 66c8172 Compare September 2, 2020 00:28
@chidozieononiwu
Copy link
Member Author

/check-enforcer override

@chidozieononiwu chidozieononiwu merged commit 93cb309 into Azure:master Sep 4, 2020
sima-zhu pushed a commit to sima-zhu/azure-sdk-tools that referenced this pull request Dec 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants