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

GH2429: Add build system provider property #2430

Merged
merged 3 commits into from
Jan 8, 2019

Conversation

gitfool
Copy link
Contributor

@gitfool gitfool commented Jan 4, 2019

Fixes #2429.

  • Added BuildProvider enum flags
  • Added Provider property; initialized in ctor
  • Changed IsLocalBuild property to use Provider property; initialized in ctor
  • Added AzurePipelines and AzurePipelinesHosted
  • Deprecated all things TFS and VSTS with Obsolete attribute
  • Used xUnit theories to reduce unit tests surface area

@gitfool
Copy link
Contributor Author

gitfool commented Jan 4, 2019

@devlead there are unrelated build failures, probably due to recent changes?

@devlead
Copy link
Member

devlead commented Jan 4, 2019

Seems to be a code style issue

- Unit\Build\BuildSystemTests.cs(1232,1): error SA1507: Code should not contain multiple blank lines in a row 

If you do a release build they should appear locally too.

@gitfool gitfool force-pushed the gh2429 branch 2 times, most recently from 8393d12 to 8b2bda7 Compare January 5, 2019 01:07
@gitfool gitfool changed the title [WIP] GH2429: Add helper method to get build system provider name GH2429: Add build system provider property Jan 5, 2019
@gitfool
Copy link
Contributor Author

gitfool commented Jan 5, 2019

@devlead I've finalized my changes and removed the [WIP] label. Ready for review.

.Where(property => property.Name.StartsWith("IsRunningOn") && property.GetValue(this) is bool value && value)
.Select(property =>
{
var provider = property.Name.Substring(11);
Copy link

Choose a reason for hiding this comment

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

maybe .Substring("IsRunningOn".Length) for clarity?

Copy link
Contributor Author

@gitfool gitfool Jan 5, 2019

Choose a reason for hiding this comment

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

Should be obvious, but mostly harmless, so done!

I also removed mapping TFS & VSTS to TFBuild as that loses information. Ideally, all things TFS would be renamed to AzurePipelines, but that's a breaking change so should be avoided, presumably.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, if look at the Azure Pipelines variables, they're still referring to vstfs:// in the uri and vsts in paths, so probably best to let the new name soak for a while before even considering renaming anything. 😛

@gitfool gitfool force-pushed the gh2429 branch 2 times, most recently from e08eb3b to d5e494f Compare January 7, 2019 20:35
@@ -114,6 +115,12 @@ public sealed class BuildSystem
GoCD = goCDProvider;
GitLabCI = gitlabCIProvider;
TFBuild = tfBuildProvider;

Provider = typeof(BuildSystem)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using Reflection, Linq, Substrings and SingleOrDefault (which could generate exceptions in constructor if you get more than one property), it would probably be more clear to just use an enum and the booleans. Roughly something like

[Flags]
public enum BuildProvider
{
   Local = 0,
   AppVeyor = 1,
   TeamCity = 2,
   MyGet = 4,
   Bamboo = 8,
   ContinuaCI = 16,
   Jenkins = 32,
   Bitrise = 64,
   TravisCI = 128,
   BitbucketPipelines = 256,
   GoCD = 512,
   GitLabCI = 1024,
   AzurePipelines = 2048,
   AzurePipelinesHosted = 4096 
}

a used by a provider property

public BuildProvider Provider { get; }

assigned in constructor something like this

Provider =  (AppVeyor.IsRunningOnAppVeyor ? BuildProvider.AppVeyor : BuildProvider.Local)
        | (TeamCity.IsRunningOnTeamCity ? BuildProvider.TeamCity : BuildProvider.Local)
        | (MyGet.IsRunningOnMyGet ? BuildProvider.MyGet : BuildProvider.Local)
        | (Bamboo.IsRunningOnBamboo ? BuildProvider.Bamboo : BuildProvider.Local)
        | (ContinuaCI.IsRunningOnContinuaCI ? BuildProvider.ContinuaCI : BuildProvider.Local)
        | (Jenkins.IsRunningOnJenkins ? BuildProvider.Jenkins : BuildProvider.Local)
        | (Bitrise.IsRunningOnBitrise ? BuildProvider.Bitrise : BuildProvider.Local)
        | (TravisCI.IsRunningOnTravisCI ? BuildProvider.TravisCI : BuildProvider.Local)
        | (BitbucketPipelines.IsRunningOnBitbucketPipelines ? BuildProvider.BitbucketPipelines : BuildProvider.Local)
        | (GoCD.IsRunningOnGoCD ? BuildProvider.GoCD : BuildProvider.Local)
        | (GitLabCI.IsRunningOnGitLabCI ? BuildProvider.GitLabCI : BuildProvider.Local)
        | (TFBuild.IsRunningOnTFS ? BuildProvider.AzurePipelines : BuildProvider.Local)
        | (TFBuild.IsRunningOnVSTS ? BuildProvider.AzurePipelinesHosted : BuildProvider.Local);

IsLocalBuild = Provider == BuildProvider.Local;

Then IsLocalBuild could be something like

public bool IsLocalBuild { get; }

If for some reason you would run an AzurePipelines agent on i.e. AppVeyor 😎 then

Information(
   new {
      BuildSystem.Provider,
      BuildSystem.IsLocalBuild
   }
   );

would output

{ Provider = AppVeyor, AzurePipelines, IsLocalBuild = False }

and not through an exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An enum would be better, but flags doesn’t seem right and adds its own complexity. Surely we can assume the build providers, including local, are mutually exclusive? (That’s why I used SingleOrDefault.)

Also, looks like you’re agreeable to rename VSTS to AzurePipelines (for hosted)? Is there an equivalent of TFS though; i.e. is there on-premise version of Azure Pipelines?

Copy link
Member

Choose a reason for hiding this comment

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

Next version of TFS is renamed to Azure DevOps Server 2019 ( https://azure.microsoft.com/en-us/services/devops/server/ ) and will have the Azure Pipelines branding for build and release.

Yes there should just be one, but I rather it not fail in the constructor, as it's environment variable it's just strings, strings can't be trusted :), and we don't fail today if multiple providers would return true, changing that behavior could break peoples existing builds.
I.e. if some one does integration tests with a Jenkins container on Azure Pipelines it wouldn't have TF_BUILD variable today, but it will in 2-3 weeks (when the fixed issue you raised is in prod). And that would "correctly" light two build systems, where there previously was one.

IMHO a flags enum is less comlpex than reflection, linq and string indexing and if there would be 2 or more providers identified, even if that shouldn't be the case then Provider would reflect that, not me null/default/throw exception.

@gitfool
Copy link
Contributor Author

gitfool commented Jan 8, 2019

@devlead you make some good points so I refactored everything as you suggested and force pushed.

Copy link
Member

@devlead devlead left a comment

Choose a reason for hiding this comment

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

LGTM 👍 I'll merge once CI passes.

@devlead devlead merged commit b9b8ae6 into cake-build:develop Jan 8, 2019
@devlead
Copy link
Member

devlead commented Jan 8, 2019

@gitfool your changes have been merged, thanks for your contribution 👍

@gitfool gitfool deleted the gh2429 branch January 8, 2019 23:17
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.

3 participants