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

Always set exit code #282

Merged
merged 7 commits into from Nov 15, 2014
Merged

Always set exit code #282

merged 7 commits into from Nov 15, 2014

Conversation

asbjornu
Copy link
Member

When GitVersion.exe is passed invalid arguments, it just outputs the help screen and returns 0 as its exit code. This just caused mayhem in a TeamCity build I tested out GitVersion in, because the whole build executed from start to finish without any step failing. Not even the NuGet Pack step failed, even though it used %system.GitVersion.FullSemVer% (which was just an empty string). You might blame JetBrains for this; the NuGet Pack build step should fail if its "Version" input is set to a variable and that variable returns null.

This PR fixes this problem by refactoring the Program class a bit so it invokes a Run() method that always returns an exit code and then always sets that exit code with Environment.Exit(exitCode).

I've also added a test that verifies the exit code in case of invalid arguments, which necessitated a little refactoring of the GitVersionHelper class.

PS: This PR also includes #280.

…h seems to fix a problem where all tests are run in fast succession.
… code which is then always set on the Environment.
…hen the program is run on a build server, the build fails when GitVersion fails.
… to GitVersionHelper in preparation for a test that needs to pass in invalid arguments.
…sed to GitVersionExe. The exit code should be 1 (not 0).
@gep13
Copy link
Member

gep13 commented Oct 27, 2014

@asbjornu Did you see the note above about the failed build from the merge of this Pull Request. Can you take a look and get them fixed up? The contributors here likely won't accept the Pull Request until these are fixed.

@asbjornu
Copy link
Member Author

Yea, @gep13, I noticed that. I've just registered an account and will fix the inspection errors ASAP. The problems noted are:

GitVersionExe.Tests: ArgumentBuilder.cs (5)

  1. 16: Accessor 'Exec.get' can be made private
  2. 18: Accessor 'ExecArgs.get' can be made private
  3. 20: Accessor 'ProjectFile.get' can be made private
  4. 22: Accessor 'ProjectArgs.get' can be made private
  5. 28: Accessor 'AdditionalArguments.get' can be made private

@gep13
Copy link
Member

gep13 commented Oct 27, 2014

Do you have a ReSharper license? If so, it is the same inspections that are being run on the TeamCity instance, i.e. you can test for them before making the Pull Request.

If not, there is a command line version of InspectCode, which is what ReSharper is using, that you can use:

choco install resharper-clt

Would get you them.

@JakeGinnivan
Copy link
Contributor

From memory @distantcam made this change. Cam, what are your thoughts?

@asbjornu
Copy link
Member Author

@gep13 Yea, I have ReSharper. But I don't see the same code inspection problems there as those TeamCity complained about. Anyway, the problems are fixed now so this should be safe to merge.

@JakeGinnivan
Copy link
Contributor

@asbjornu you need to have solution wide analysis turned on to see those errors I think

@gep13
Copy link
Member

gep13 commented Oct 28, 2014

@asbjornu you need to have solution wide analysis turned on to see those errors I think

Yes, this is exactly what I was going to say.

image

@asbjornu
Copy link
Member Author

Yea, that setting is turned on through the GitVersion.sln.DotSettings file.

@JakeGinnivan
Copy link
Contributor

@distantcam @SimonCropp @andreasohlund what are your thoughts on this change.

@andreasohlund
Copy link
Contributor

Seems very resonable to me

@distantcam
Copy link
Contributor

👍

JakeGinnivan added a commit that referenced this pull request Nov 15, 2014
@JakeGinnivan JakeGinnivan merged commit b77694a into GitTools:master Nov 15, 2014
@JakeGinnivan JakeGinnivan added this to the 1.3.4 milestone Nov 15, 2014
@asbjornu asbjornu deleted the always-set-exit-code branch November 17, 2014 08:37
@SimonCropp SimonCropp added the bug label Dec 16, 2014
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.

None yet

6 participants