Skip to content

WIP Migrate aspire 9#63

Closed
Alirexaa wants to merge 6 commits intoCommunityToolkit:mainfrom
Alirexaa:migrate-aspire-9
Closed

WIP Migrate aspire 9#63
Alirexaa wants to merge 6 commits intoCommunityToolkit:mainfrom
Alirexaa:migrate-aspire-9

Conversation

@Alirexaa
Copy link
Copy Markdown
Member

@Alirexaa Alirexaa commented Oct 4, 2024

Closes #56

This pull request includes several updates and improvements across multiple files, primarily focusing on upgrading the Aspire SDK version, removing unnecessary workload installations, and adding new NuGet configurations. Below is a summary of the most important changes:

Workflow Updates:

  • Removed the installation of the Aspire workload in the dotnet-ci.yml, dotnet-main.yml, and dotnet-release.yml files. (.github/workflows/dotnet-ci.yml [1] .github/workflows/dotnet-main.yml [2] .github/workflows/dotnet-release.yml [3]

SDK Version Upgrades:

  • Updated the AspireVersion from 8.2.0 to 9.0.0-preview.4.24504.1 in Directory.Build.props. (Directory.Build.props Directory.Build.propsL10-R10)
  • Added the Aspire.AppHost.Sdk with version 9.0.0-preview.4.24477.2 to various project files. (examples/java/Aspire.CommunityToolkit.Hosting.Java.AppHost/Aspire.CommunityToolkit.Hosting.Java.AppHost.csproj [1] examples/nodejs-ext/Aspire.CommunityToolkit.Hosting.NodeJS.Extensions.AppHost/Aspire.CommunityToolkit.Hosting.NodeJS.Extensions.AppHost.csproj [2] examples/ollama/Aspire.CommunityToolkit.Hosting.Ollama.AppHost/Aspire.CommunityToolkit.Hosting.Ollama.AppHost.csproj [3] examples/swa/Aspire.CommunityToolkit.StaticWebApps.AppHost/Aspire.CommunityToolkit.StaticWebApps.AppHost.csproj [4]

Configuration Additions:

  • Added a new nuget.config file with specific package sources and mappings for Aspire packages. (nuget.config nuget.configR1-R15)

Test File Updates:

  • Added using Aspire.Hosting; to several test files to ensure proper namespace usage. (tests/Aspire.CommunityToolkit.Hosting.Java.Tests/ContainerResourceCreationTests.cs [1] tests/Aspire.CommunityToolkit.Hosting.Java.Tests/ExecutableResourceCreationTests.cs [2] tests/Aspire.CommunityToolkit.Hosting.NodeJS.Extensions.Tests/ResourceCreationTests.cs [3]

PR Checklist

  • Created a feature/dev branch in your fork (vs. submitting directly from a commit on main)
  • Based off latest main branch of toolkit
  • PR doesn't include merge commits (always rebase on top of our main, if needed)
  • New integration
    • Docs are written
    • Added description of major feature to project description for NuGet package (4000 total character limit, so don't push entire description over that)
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Contains NO breaking changes
  • Every new API (including internal ones) has full XML docs
  • Code follows all style conventions

Other information

@Alirexaa Alirexaa marked this pull request as ready for review October 4, 2024 12:45
@Alirexaa Alirexaa requested a review from aaronpowell as a code owner October 4, 2024 12:45
@Alirexaa
Copy link
Copy Markdown
Member Author

Alirexaa commented Oct 4, 2024

Why ci action not running?

@aaronpowell aaronpowell changed the title Migrate aspire 9 WIP Migrate aspire 9 Oct 5, 2024
@aaronpowell
Copy link
Copy Markdown
Member

Hmm I wonder if CI isn't liking that it came from a fork, although I haven't seen problems from other forks.

Given that we want to hold this for a while, I've renamed the PR to start with WIP which will cause the WIP app to prevent it from merging.

@aaronpowell aaronpowell mentioned this pull request Oct 5, 2024
10 tasks
@Alirexaa
Copy link
Copy Markdown
Member Author

Alirexaa commented Oct 6, 2024

Hmm I wonder if CI isn't liking that it came from a fork, although I haven't seen problems from other forks.

Given that we want to hold this for a while, I've renamed the PR to start with WIP which will cause the WIP app to prevent it from merging.

Could you check Fork pull request workflows is enable?
see https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository#enabling-workflows-for-forks-of-private-repositories

@aaronpowell
Copy link
Copy Markdown
Member

Hmm I wonder if CI isn't liking that it came from a fork, although I haven't seen problems from other forks.
Given that we want to hold this for a while, I've renamed the PR to start with WIP which will cause the WIP app to prevent it from merging.

Could you check Fork pull request workflows is enable? see https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository#enabling-workflows-for-forks-of-private-repositories

Ah, no it's not, and I also can't enable it for some reason.

You should have permission to push that branch here, so maybe do that instead.

@@ -1,5 +1,6 @@
<Project Sdk="Microsoft.NET.Sdk">

<Sdk Name="Aspire.AppHost.Sdk" Version="9.0.0-preview.4.24477.2"/>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we make that a variable?

Comment thread nuget.config
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't we need a fallback source configured for nuget or does it automatically fallback?

Comment on lines +1 to +2
using Aspire.Hosting;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this needed? If not:

Suggested change
using Aspire.Hosting;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

After upgrade to 9 preview build failed. I fixed build by adding this namespace.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Weird, feel like that might be a breaking change - what was the error?

@@ -1,3 +1,4 @@
using Aspire.Hosting;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this needed? If not:

Suggested change
using Aspire.Hosting;

Comment on lines +1 to +2
using Aspire.Hosting;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this needed? If not:

Suggested change
using Aspire.Hosting;

@aaronpowell aaronpowell added this to the aspire-9 milestone Oct 7, 2024
@aaronpowell aaronpowell added enhancement New feature or request integration A new .NET Aspire integration labels Oct 7, 2024
@aaronpowell
Copy link
Copy Markdown
Member

I'm going to close this in favour of #87 which brings the branch into the Community Toolkit repo. That way we'll be able to branch from the branch within the repo and work on the sub-tasks to get Aspire 9 support tackled (such as using it for #55)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request integration A new .NET Aspire integration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Aspire 9 support

2 participants