Skip to content

Conversation

@mmitche
Copy link
Member

@mmitche mmitche commented Oct 19, 2021

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

To double check:

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
@mmitche mmitche requested a review from alexperovich October 19, 2021 23:18
@mmitche
Copy link
Member Author

mmitche commented Oct 19, 2021

@alexperovich This bug I think is going to block RTM stabilization until fixed (tools feeds specifically)

Copy link
Member

@alexperovich alexperovich left a comment

Choose a reason for hiding this comment

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

I collapsed these down to avoid repetition. It's sad that we have to put it back, but this makes sense.

@mmitche
Copy link
Member Author

mmitche commented Oct 19, 2021

I collapsed these down to avoid repetition. It's sad that we have to put it back, but this makes sense.

The complexity would end up somewhere. Either there or in some fancy logic to go and find the "all" cases and change them. I think this is a bit better.

@mmitche mmitche merged commit cafbb54 into dotnet:main Oct 20, 2021
@mmitche mmitche deleted the fixup-isolated-feed-breakout branch October 20, 2021 02:53
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.

2 participants