-
Notifications
You must be signed in to change notification settings - Fork 651
Control when an attempt is made to upload releasenotes.md #1093
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
Conversation
- Only do this when on the master branch of main GitVersion repo, and when it is NOT a PullRequest - Ensure that releasenotes.md exists prior to attempting upload
@pascalberger @asbjornu let me know what you think of this... |
Thanks @gep13! LGTM |
Now just to wait a few extra minutes for Travis 😄 |
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.
Besides the commited .vscode
folder, this looks great! Thanks for the work, @gep13! ❤️
"externalConsole": false | ||
} | ||
] | ||
} |
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.
Should anything in the .vscode
folder be committed?
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.
@asbjornu I wasn't aware of any rule that it shouldn't be. That file is an easy way to share the required launch command for debugging build.cake file in VSCode.
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.
Yes, these are the settings for debugging and should be committed (one of the three file: settings.json
, tasks.json
and launch.json
which should be under version control)
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.
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.
@pascalberger nice, I can add that to the gitignore file. Just away to sign off for the night though, so if Travis ever finishes, and we are all happy, feel free to merge, and I can fix that later.
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.
@gep13 Unfortunately a test case, not releated to the release notes, failed on OS X: https://travis-ci.org/GitTools/GitVersion/jobs/177131223.
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.
@gep13, @pascalberger: Okay, good to know. I agree that it should be in the .gitignore
just to make it explicit. 🙂
The set_PriorityClass
problem is hopefully fixed by GitTools/GitTools.Core#38, but we need to get that released somehow. Kind of related to this PR, but for different reasons and another repository. 😄
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.
Mmh, as long as this is not fixed, we can't merge this PR, except by overriding the required status checks. So to cleanly merge this we would need to have the GitTools.Core release and update GitVersion to the new GitTools.Core version in this PR.
Relates to #1088 |
No description provided.