cake icon indicating copy to clipboard operation
cake copied to clipboard

TFBuild.IsRunningOnVSTS is confusing when using own build agent

Open Cheesebaron opened this issue 8 years ago • 5 comments

What You Are Seeing?

I have a VSTS build agent, which is triggering a cake build on GitHub commits.

I tried using the TFBuild.IsRunningOnVSTS to check if this was a build running on VSTS, as I am also using AppVeyor.

What is Expected?

I would have expected TFBuild.IsRunningOnVSTS to return true. It returned false.

This seems to be per design as seen in the TFBuildProvider

What version of Cake are you using?

Latest

I discussed this briefly with @patriksvensson on Gitter and we kind of agreed that the IsRunningOnVSTS and IsRunningOnTFS properties could be a bit misleading.

@patriksvensson suggested a possible rename:

IsRunningOnVSTS -> IsRunningOnSharedAgent IsRunningOnTFS -> IsRunningOnOwnAgent

To me this would be much more clear as VSTS can both run shared and own agents.

@agc93 you seem to be the original author of this feature, what do you think about this?

Cheesebaron avatar Apr 23 '17 22:04 Cheesebaron

I don't think we should rename them, but add something similar to what is suggested to reduce confusion. I'm very open for suggestions for naming of this 😄

patriksvensson avatar Apr 23 '17 22:04 patriksvensson

Okay so jumping in on this there's a difference between VSTS/TFS and hosted agent/own agent, since it's (theoretically) possible to have any combination of those.

So, short version is that, yeah this behaviour is probably not quite right since VSTS + self-hosted agent is a perfectly valid combo. You can always check whether you're on a hosted agent using the TFBuild.Environment.Agent.IsHosted property. For the system, my thinking is to probably add an additional IsRunningOnTFBuild to encompass both TFS and VSTS (and any agent type).

Would that suit your use case @Cheesebaron ? Thoughts @patriksvensson ?

EDIT: I'll add a note to the documentation for this too, since I think it probably doesn't warrant a breaking change, personally.

agc93 avatar Apr 23 '17 23:04 agc93

Any thoughts/feedback before I start on this @Cheesebaron or @patriksvensson ? I've talked to Microsoft about it, so I'll try and improve the logic as well as adding that new property.

agc93 avatar Apr 27 '17 01:04 agc93

I don't really care myself whether this is a breaking change or not. However, for people using this, they probably want their scripts to continue to work.

IsHosted is more clear.

If there is not actual distinction between IsRunningOnVSTS and IsRunningOnTFS other than one is hosted and the other is not, I would try to remove that confusion entirely.

Cheesebaron avatar Apr 27 '17 08:04 Cheesebaron

Okay so I don't think removing IsRunningOnVSTS/IsRunningOnTFS is either a) the right call or b) something we can change at this point (that's one hell of a breaking change).

As discussed, you can already use TFBuild.Environment.Agent.IsHosted to confirm if you're on a hosted agent, regardless of the IsRunningOn* properties.

I will look into improving the logic of IsRunningOn and PR it when I get a chance.

agc93 avatar May 01 '17 16:05 agc93