-
Notifications
You must be signed in to change notification settings - Fork 389
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
Fix dependency node when no TargetFramework(s) property exists in the project #6975
Conversation
Fixes dotnet#6974 Currently, the logic is to construct these subscriptions _only_ when the `TargetFramework` or `TargetFrameworks` properties change. For a regular .NET project this is fine as that property will be defined, and when it is first observed it will trigger the subscriptions that allow the Dependencies node to function correctly. For a project type that does not define these properties (i.e. WAP projects) this logic is incorrect. This fix ensures a `AggregateCrossTargetProjectContext` when the current instance is null.
@@ -311,7 +311,8 @@ private void DisposeCore() | |||
private Task OnConfiguredProjectEvaluatedAsync(IProjectVersionedValue<IProjectSubscriptionUpdate> e) | |||
{ | |||
// If "TargetFrameworks" property has changed, we need to refresh the project context and subscriptions. | |||
if (HasTargetFrameworksChanged()) | |||
// If no context exists yet, create one. | |||
if (HasTargetFrameworksChanged() || _context.Current == null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does the old behavior result in a yellow bar warning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the old behaviour fails silently. It would never create the necessary data flow subscriptions, so the Dependencies node would appear empty indefinitely.
Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:
These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check. Give feedback on thisFrom the bot dev teamWe've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments. Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin. |
Fixes #6974
Currently, the logic is to construct these subscriptions only when the
TargetFramework
orTargetFrameworks
properties change.For a regular .NET project this is fine as that property will be defined, and when it is first observed it will trigger the subscriptions that allow the Dependencies node to function correctly.
For a project type that does not define these properties (i.e. WAP projects) this logic is incorrect.
This fix ensures a
AggregateCrossTargetProjectContext
when the current instance is null.Microsoft Reviewers: Open in CodeFlow