-
Notifications
You must be signed in to change notification settings - Fork 652
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
Add assembly-file-versioning-format with support for environment variables #1385
Add assembly-file-versioning-format with support for environment variables #1385
Conversation
Thanks a lot for your work. I gave it a try and it works like a charm (just remember to call I would love a comment from the maintainers whether this approach has chances to get merged. @asbjornu, you already gave thumbs up on my comment in #1157... Please tell us if you request further changes to the implementation, format string syntax, or the like. |
@jbaehr You are welcome and thanks for testing out the changes :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks really promising. I just have a few comments, so please address or debate them before we move forward. Thanks for doing this!
…rmatWith, updated docs
@asbjornu You are welcome. I have addressed some of your review comments and provided my opinion on others. In my opinion, we have the functionality in place that addresses the theme of this issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good now, minus one question. Thanks!
…for readability, updated the tests & docs
You are welcome. I have included the changes for the null propagation operator readability and updated some test cases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few new questions, otherwise this looks good!
@asbjornu I would like to address all the changes you request in one go. This is getting repetitive. The review before this should have had these comments because these changes have been there before the last commit. I will give you some time to make up your mind about the PR as a whole and once you have reviewed all the code let me know, i will address your comments. |
@ruhullahshah: Sorry about that. I was sure the latest review was for new code. I should of course have reviewed that in the first go, but if I remember correctly, I had limited time when I did the first review so it wasn't as thorough as I would have liked. |
Excellent. Thanks for this! |
You are welcome :) |
Hi Team, Following is my gitversion config next-version: 1.0.0
assembly-versioning-scheme: MajorMinorPatch
assembly-versioning-format: '{Major}.{Minor}.{Patch}.{env:BUILD_COUNTER??0}'
assembly-file-versioning-scheme: MajorMinorPatch
continuous-delivery-fallback-tag: ''
mode: ContinuousDeployment
branches:
release:
tag: ''
develop:
tag: 'dev'
master:
tag: 'master'
ignore:
sha: []
merge-message-formats: {} In Team City project I have an environment variable
After the build this is what I get Can you please let me know what I am doing wrong here? Or what do I need to do to make sure env:BUILD_COUNTER gets substituted with the correct value? Thank you |
@vishalvatsal The implementation in this PR explicitly requires spaces around the Note however, that the currently latest GitVersion release features an enhanced implementation, that further allows fallback strings for regular GitVersion variables, too, and also allows for quoted fallback strings containing special characters. cf. #2179 |
Thanks a ton @jbaehr. It works now. |
Fix for issue #1366
Implementation methodology same as suggested by @jbaehr in #1157