Skip to content

Conversation

joperezr
Copy link
Member

@joperezr joperezr commented Sep 5, 2024

Description

This would walk back on a decision we took in 8.2.0 where we forced people to reference the 8.2.0 AppHost package when moving to the new workload, and instead allow for backward compatibility preserving the old behavior where, if that is the case, then the version of dashboard and DCP people would get would match the version of the workload they have installed.

The intention would be to backport this to release/8.2 branch so it can be serviced in 8.2.1

cc: @DamianEdwards @davidfowl @eerhardt @maddymontaquila @radical

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?
    • Yes
      • Link to aspire-docs issue:
    • No
Microsoft Reviewers: Open in CodeFlow

@ghost ghost added the area-integrations Issues pertaining to Aspire Integrations packages label Sep 5, 2024
@maddymontaquila
Copy link
Member

do ittttt. there's no way this is going to break CI systems again, right? 😄

@joperezr
Copy link
Member Author

joperezr commented Sep 5, 2024

do ittttt. there's no way this is going to break CI systems again, right? 😄

Well now you've jinxed it 🤭....jk.

The plan is that this change would undo the regression, and allow people to remove the manual workarounds they had to do to pin an older version of the workload.

@KennethHoff
Copy link

This is a temporary solution until 9.0, right? iiuc 9.0 will no longer require installing the workload, as the only thing it will do is keep the templates up-to-date. Is that the right understanding?

@DamianEdwards
Copy link
Member

@KennethHoff correct

@davidfowl davidfowl added area-tooling and removed area-integrations Issues pertaining to Aspire Integrations packages labels Sep 7, 2024
@mitchdenny
Copy link
Member

Wouldn't this need to go only into the 8.2 branch instead of main?

@joperezr
Copy link
Member Author

joperezr commented Sep 9, 2024

The plan is to backport to 8.2 once this is merged. We still have to do workload authoring changes for 9 which will modify this code again in main branch, so for now we should have it in both places.

@joperezr
Copy link
Member Author

@DamianEdwards @eerhardt @radical any feedback here? Would be good to get this one closed so we can open the backport PR

@radical
Copy link
Member

radical commented Sep 11, 2024

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

}

[Fact]
public async Task EnsureProjectsReferencing8_1_0WorkloadCanBuild()
Copy link
Member

Choose a reason for hiding this comment

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

Should this be named EnsureProjectsReferencing8_1_0AppHostWithNewerWorkloadCanBuild?

@DamianEdwards
Copy link
Member

@DamianEdwards @eerhardt @radical any feedback here? Would be good to get this one closed so we can open the backport PR

Conceptually good from my side.

</PropertyGroup>

<!-- At this point, we should have the version either by CPM or PackageReference, so we fail if not. -->
<Error Condition="'$(_AppHostVersion)' == '' or $([MSBuild]::VersionLessThan('$(_AppHostVersion)', '8.2.0'))"
Text="$(MSBuildProjectName) is a .NET Aspire AppHost project that needs a package reference to Aspire.Hosting.AppHost version 8.2.0 or above to work correctly.$(__CurrentAppHostVersionMessage)" />
<Error Condition="'$(_AppHostVersion)' == ''"
Copy link
Member

Choose a reason for hiding this comment

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

A test for this would be good too. It doesn't have to be in this PR.
Also, the same for both cases but with CPM?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add more tests to this whole scenario with the changes on the Aspire Workload for 9.0

Copy link
Member

@radical radical left a comment

Choose a reason for hiding this comment

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

Other than the feedback, LGTM 👍

@joperezr joperezr enabled auto-merge (squash) September 12, 2024 22:02
@joperezr joperezr merged commit 99b713f into dotnet:main Sep 12, 2024
11 checks passed
@joperezr
Copy link
Member Author

/backport to release/8.2

Copy link
Contributor

Started backporting to release/8.2: https://github.com/dotnet/aspire/actions/runs/10853030465

@github-actions github-actions bot locked and limited conversation to collaborators Oct 14, 2024
@joperezr joperezr deleted the AllowTargeting810 branch April 1, 2025 17:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants