From cdffbe64e952a120a5de67d561c7c0409b070462 Mon Sep 17 00:00:00 2001 From: Matt Mitchell Date: Tue, 19 Oct 2021 15:33:38 -0700 Subject: [PATCH] Fix isolated feed issue When the new target feed specifications were created, they collapsed down the feed specs for shipping and non-shipping package asset selection in cases where the target would be the same feed. This subtlety was required though, since in case of a stable build, we want just the shipping packages to go to a separate, newly created feed. The current code correct substitutes this if the TargetFeedSpecifications are broken out for ShippingOnly and NonShippingOnly, but there are a number of channels that did not break them out. Instead, it tried to send shipping packages to the isolated feed, and all packages to the non-isolated feed. To fix this, separate out the TargetFeedSpecifications and put a check into the constructor to ensure that this can't be done accidentally. Also - Do a little refactoring for clarity. - Add a test to ensure that this throws --- .../SetupTargetFeedConfigV3Tests.cs | 13 +++ .../src/model/PublishingConstants.cs | 21 ++-- .../src/model/SetupTargetFeedConfigV3.cs | 102 +++++++++++------- .../src/model/TargetChannelConfig.cs | 9 ++ 4 files changed, 99 insertions(+), 46 deletions(-) diff --git a/src/Microsoft.DotNet.Build.Tasks.Feed.Tests/SetupTargetFeedConfigV3Tests.cs b/src/Microsoft.DotNet.Build.Tasks.Feed.Tests/SetupTargetFeedConfigV3Tests.cs index dd11607ea0f..84d2a9d15e9 100644 --- a/src/Microsoft.DotNet.Build.Tasks.Feed.Tests/SetupTargetFeedConfigV3Tests.cs +++ b/src/Microsoft.DotNet.Build.Tasks.Feed.Tests/SetupTargetFeedConfigV3Tests.cs @@ -437,5 +437,18 @@ private bool AreEquivalent(List expectedItems, List new TargetFeedSpecification(new TargetFeedContentType[] { TargetFeedContentType.Package }, "FooFeed", AssetSelection.All); + shouldFail.Should().Throw(); + + Action shouldPassShippingOnly = () => new TargetFeedSpecification(new TargetFeedContentType[] { TargetFeedContentType.Package }, "FooFeed", AssetSelection.ShippingOnly); + shouldPassShippingOnly.Should().NotThrow(); + + Action shouldPassNonShippingOnly = () => new TargetFeedSpecification(new TargetFeedContentType[] { TargetFeedContentType.Package }, "FooFeed", AssetSelection.NonShippingOnly); + shouldPassNonShippingOnly.Should().NotThrow(); + } } } diff --git a/src/Microsoft.DotNet.Build.Tasks.Feed/src/model/PublishingConstants.cs b/src/Microsoft.DotNet.Build.Tasks.Feed/src/model/PublishingConstants.cs index b4b55fe42ef..6d44e394605 100644 --- a/src/Microsoft.DotNet.Build.Tasks.Feed/src/model/PublishingConstants.cs +++ b/src/Microsoft.DotNet.Build.Tasks.Feed/src/model/PublishingConstants.cs @@ -155,7 +155,8 @@ public enum BuildQuality private static TargetFeedSpecification[] DotNet31BlazorFeeds = { - (TargetFeedContentType.Package, FeedDotNet31Blazor), + (TargetFeedContentType.Package, FeedDotNet31Blazor, AssetSelection.ShippingOnly), + (TargetFeedContentType.Package, FeedDotNet31Blazor, AssetSelection.NonShippingOnly), (InstallersAndSymbols, FeedInternalForInstallers), (TargetFeedContentType.Checksum, FeedInternalForChecksums), }; @@ -204,35 +205,40 @@ public enum BuildQuality private static TargetFeedSpecification[] DotNetEngFeeds = { - (TargetFeedContentType.Package, FeedDotNetEng), + (TargetFeedContentType.Package, FeedDotNetEng, AssetSelection.ShippingOnly), + (TargetFeedContentType.Package, FeedDotNetEng, AssetSelection.NonShippingOnly), (InstallersAndSymbols, FeedForInstallers), (TargetFeedContentType.Checksum, FeedForChecksums), }; private static TargetFeedSpecification[] DotNetToolsFeeds = { - (TargetFeedContentType.Package, FeedDotNetTools), + (TargetFeedContentType.Package, FeedDotNetTools, AssetSelection.ShippingOnly), + (TargetFeedContentType.Package, FeedDotNetTools, AssetSelection.NonShippingOnly), (InstallersAndSymbols, FeedForInstallers), (TargetFeedContentType.Checksum, FeedForChecksums), }; private static TargetFeedSpecification[] DotNetToolsInternalFeeds = { - (TargetFeedContentType.Package, FeedDotNetToolsInternal), + (TargetFeedContentType.Package, FeedDotNetToolsInternal, AssetSelection.ShippingOnly), + (TargetFeedContentType.Package, FeedDotNetToolsInternal, AssetSelection.NonShippingOnly), (InstallersAndSymbols, FeedInternalForInstallers), (TargetFeedContentType.Checksum, FeedInternalForChecksums), }; private static TargetFeedSpecification[] DotNetExperimentalFeeds = { - (TargetFeedContentType.Package, FeedDotNetExperimental), + (TargetFeedContentType.Package, FeedDotNetExperimental, AssetSelection.ShippingOnly), + (TargetFeedContentType.Package, FeedDotNetExperimental, AssetSelection.NonShippingOnly), (InstallersAndSymbols, FeedForInstallers), (TargetFeedContentType.Checksum, FeedForChecksums), }; private static TargetFeedSpecification[] GeneralTestingFeeds = { - (TargetFeedContentType.Package, FeedGeneralTesting), + (TargetFeedContentType.Package, FeedGeneralTesting, AssetSelection.ShippingOnly), + (TargetFeedContentType.Package, FeedGeneralTesting, AssetSelection.NonShippingOnly), (InstallersAndSymbols, FeedForInstallers), (TargetFeedContentType.Checksum, FeedForChecksums), (InstallersAndSymbols, FeedStagingForInstallers), @@ -241,7 +247,8 @@ public enum BuildQuality private static TargetFeedSpecification[] GeneralTestingInternalFeeds = { - (TargetFeedContentType.Package, FeedGeneralTestingInternal), + (TargetFeedContentType.Package, FeedGeneralTestingInternal, AssetSelection.ShippingOnly), + (TargetFeedContentType.Package, FeedGeneralTestingInternal, AssetSelection.NonShippingOnly), (InstallersAndSymbols, FeedInternalForInstallers), (TargetFeedContentType.Checksum, FeedInternalForChecksums), (InstallersAndSymbols, FeedStagingInternalForInstallers), diff --git a/src/Microsoft.DotNet.Build.Tasks.Feed/src/model/SetupTargetFeedConfigV3.cs b/src/Microsoft.DotNet.Build.Tasks.Feed/src/model/SetupTargetFeedConfigV3.cs index 999a3c70c58..2296192100b 100644 --- a/src/Microsoft.DotNet.Build.Tasks.Feed/src/model/SetupTargetFeedConfigV3.cs +++ b/src/Microsoft.DotNet.Build.Tasks.Feed/src/model/SetupTargetFeedConfigV3.cs @@ -86,46 +86,14 @@ public override List Setup() private IEnumerable Feeds() { + // If the build is stable, we need to create two new feeds (if not provided) + // that can contain stable packages. These packages cannot be pushed to the normal + // feeds specified in the feed config because it would mean pushing the same package more than once + // to the same feed on successive builds, which is not allowed. if (IsStableBuild) { - if (string.IsNullOrEmpty(StablePackagesFeed)) - { - var packagesFeedTask = new CreateAzureDevOpsFeed() - { - BuildEngine = BuildEngine, - IsInternal = IsInternalBuild, - AzureDevOpsPersonalAccessToken = AzureDevOpsFeedsKey, - RepositoryName = RepositoryName, - CommitSha = CommitSha - }; - - if (!packagesFeedTask.Execute()) - { - throw new Exception($"Problems creating an AzureDevOps feed for repository '{RepositoryName}' and commit '{CommitSha}'."); - } - - StablePackagesFeed = packagesFeedTask.TargetFeedURL; - } - - if (string.IsNullOrEmpty(StableSymbolsFeed)) - { - var symbolsFeedTask = new CreateAzureDevOpsFeed() - { - BuildEngine = BuildEngine, - IsInternal = IsInternalBuild, - AzureDevOpsPersonalAccessToken = AzureDevOpsFeedsKey, - RepositoryName = RepositoryName, - CommitSha = CommitSha, - ContentIdentifier = "sym" - }; - - if (!symbolsFeedTask.Execute()) - { - throw new Exception($"Problems creating an AzureDevOps (symbols) feed for repository '{RepositoryName}' and commit '{CommitSha}'."); - } - - StableSymbolsFeed = symbolsFeedTask.TargetFeedURL; - } + CreateStablePackagesFeedIfNeeded(); + CreateStableSymbolsFeedIfNeeded(); yield return new TargetFeedConfig( TargetFeedContentType.Package, @@ -152,6 +120,7 @@ private IEnumerable Feeds() filenamesToExclude: FilesToExclude, flatten: Flatten); } + foreach (var spec in _targetChannelConfig.TargetFeeds) { foreach (var type in spec.ContentTypes) @@ -163,9 +132,11 @@ private IEnumerable Feeds() continue; } } + + // If dealing with a stable build, the package feed targeted for shipping packages and symbols + // should be skipped, as it is added above. if (IsStableBuild && ((type is TargetFeedContentType.Package && spec.Assets == AssetSelection.ShippingOnly) || type is TargetFeedContentType.Symbols)) { - // stable build shipping packages and symbols were handled above continue; } @@ -213,6 +184,59 @@ private IEnumerable Feeds() } } + /// + /// Create the stable symbol packages feed if one is not already explicitly provided + /// + /// Throws if the feed cannot be created + private void CreateStableSymbolsFeedIfNeeded() + { + if (string.IsNullOrEmpty(StableSymbolsFeed)) + { + var symbolsFeedTask = new CreateAzureDevOpsFeed() + { + BuildEngine = BuildEngine, + IsInternal = IsInternalBuild, + AzureDevOpsPersonalAccessToken = AzureDevOpsFeedsKey, + RepositoryName = RepositoryName, + CommitSha = CommitSha, + ContentIdentifier = "sym" + }; + + if (!symbolsFeedTask.Execute()) + { + throw new Exception($"Problems creating an AzureDevOps (symbols) feed for repository '{RepositoryName}' and commit '{CommitSha}'."); + } + + StableSymbolsFeed = symbolsFeedTask.TargetFeedURL; + } + } + + /// + /// Create the stable packages feed if one is not already explicitly provided + /// + /// Throws if the feed cannot be created + private void CreateStablePackagesFeedIfNeeded() + { + if (string.IsNullOrEmpty(StablePackagesFeed)) + { + var packagesFeedTask = new CreateAzureDevOpsFeed() + { + BuildEngine = BuildEngine, + IsInternal = IsInternalBuild, + AzureDevOpsPersonalAccessToken = AzureDevOpsFeedsKey, + RepositoryName = RepositoryName, + CommitSha = CommitSha + }; + + if (!packagesFeedTask.Execute()) + { + throw new Exception($"Problems creating an AzureDevOps feed for repository '{RepositoryName}' and commit '{CommitSha}'."); + } + + StablePackagesFeed = packagesFeedTask.TargetFeedURL; + } + } + private string GetFeedOverride(string feed) { foreach (var prefix in FeedOverrides.Keys) diff --git a/src/Microsoft.DotNet.Build.Tasks.Feed/src/model/TargetChannelConfig.cs b/src/Microsoft.DotNet.Build.Tasks.Feed/src/model/TargetChannelConfig.cs index dced737bc3f..18e82775783 100644 --- a/src/Microsoft.DotNet.Build.Tasks.Feed/src/model/TargetChannelConfig.cs +++ b/src/Microsoft.DotNet.Build.Tasks.Feed/src/model/TargetChannelConfig.cs @@ -153,6 +153,15 @@ public static implicit operator TargetFeedSpecification((TargetFeedContentType t public TargetFeedSpecification(IEnumerable contentTypes, string feedUrl, AssetSelection assets) { + // A feed targeted for content type 'Package' may not have asset selection 'All'. + // During TargetFeedConfig creation, the default feed spec for shipping packages will be ignored and replaced with + // a separate target feed config. + + if (assets == AssetSelection.All && contentTypes.Contains(TargetFeedContentType.Package)) + { + throw new ArgumentException($"Target feed specification for {feedUrl} must have a separated asset selection 'ShippingOnly' and 'NonShippingOnly packages"); + } + ContentTypes = contentTypes.ToImmutableList(); FeedUrl = feedUrl; Assets = assets;