Skip to content
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

Update samples for preview.3 #108

Merged
merged 21 commits into from
Feb 14, 2024
Merged

Update samples for preview.3 #108

merged 21 commits into from
Feb 14, 2024

Conversation

balachir
Copy link
Contributor

@balachir balachir commented Feb 7, 2024

WIP branch for updating samples to preview.3. Make changes and/or additions here as necessary.

Fixes #90

@DamianEdwards
Copy link
Member

This won't pass the checks until preview.3 is released in its current state as it's not using the release feeds or a preview 8.0.200 SDK release. @balachir do you want to try updating the nuget.config to use the release feed and the global.json to point at the latest 8.0.200-preview SDK (8.0.200-preview.23624.5) for now to ensure this builds?

@DamianEdwards
Copy link
Member

DamianEdwards commented Feb 10, 2024

This won't pass the checks until preview.3 is released in its current state as it's not using the release feeds or a preview 8.0.200 SDK release. @balachir do you want to try updating the nuget.config to use the release feed and the global.json to point at the latest 8.0.200-preview SDK (8.0.200-preview.23624.5) for now to ensure this builds?

OK I ended up doing the above myself and now we can see what actually needs to be updated in this PR due to changes in preview.3

  • Replace calls to WithServiceBinding with calls to WithEndpoint or WithHttpEndpoint
  • Ensure library projects aren't referenced by AppHost projects, or if they're intended to be, update their metadata with IsAspireProjectResource="false"

…nce metadata when AppHost references a library project
@balachir
Copy link
Contributor Author

@DamianEdwards thanks. I've made the changes that you suggested, and I can build successfully on my machine now.

@ReubenBond
Copy link
Member

I believe this just needs the NuGet.config change reverted before merging

global.json Outdated
@@ -1,6 +1,6 @@
{
"sdk": {
"version": "8.0.100",
"version": "8.0.200-preview.23624.5",
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I was wondering about it too. Should we set the version to 8.0.200 now that the SDK is available? I'm not sure if the build agent is using this 8.0.200-preview version.

Copy link
Member

Choose a reason for hiding this comment

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

I think so, yes.

Copy link
Member

Choose a reason for hiding this comment

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

I pushed a commit to update it

@balachir
Copy link
Contributor Author

balachir commented Feb 13, 2024

I believe this just needs the NuGet.config change reverted before merging

I've reverted the NuGet.config changes now.

@davidfowl
Copy link
Member

/azp run

Copy link

No pipelines are associated with this pull request.

@ReubenBond
Copy link
Member

Do we need to add dotnet workload update && dotnet workload install aspire to the workflow yaml?

@DamianEdwards
Copy link
Member

Do we need to add dotnet workload update && dotnet workload install aspire to the workflow yaml?

It's already in the build.sh

@DamianEdwards
Copy link
Member

DamianEdwards commented Feb 13, 2024

The failure repros locally, seems there's an issue generating the manifest for the Dapr sample.

image

@ReubenBond
Copy link
Member

ReubenBond commented Feb 13, 2024

Ah, the issue is that the Dapr CLI is missing on the CI machine:
image

@ReubenBond
Copy link
Member

I added a Dapr CLI installation step to the pipeline

@DamianEdwards
Copy link
Member

@ReubenBond still failing

@ReubenBond
Copy link
Member

Yeah, I wonder if the dapr integration isn't working on Windows (which is running first and cancelling the linux build) - I'll see if it works on Linux and we can hold off on merging for now

@ReubenBond
Copy link
Member

Ok, that wasn't it. We get a different error on Linux. I tried updating to the latest DAPR CLI (the default from the action is very old). I assume it's still not finding the binary. I am trying to enable MS Build Structured Logs to the CI to see what the exact issue is now. We might end up needing to set the dapr/path env var manually

@ReubenBond
Copy link
Member

ReubenBond commented Feb 14, 2024

Looking at our code now... did this ever work? The dapr hosting package in Aspire only looks in C:\dapr\dapr.exe on Windows. For Linux, it checks ~/dapr/dapr and /usr/local/bin/dapr. The setup-dapr task doesn't support selecting the installation directory (it outputs a dapr-path var).

I think we need to change the Aspire.Hosting.Dapr package to either check the PATH or allow specifying the Dapr installation directory via an env var. I opened dotnet/aspire#2219 to track.

@ReubenBond
Copy link
Member

I'm tempted to say that we should either merge without CI or disable the Dapr sample from building in CI until dotnet/aspire#2219 is resolved and we have updated to a version with that (i.e, Aspire preview 4).
cc @philliphoff - perhaps you know how we might resolve this

@davidfowl
Copy link
Member

Skip the tests for the dapr sample

@joperezr
Copy link
Member

Looks like this is not updating all of the packages that were shipped yesterday, instead it is only updating the aspire ones. I can push another commit to make sure everything is up-to-date

Copy link
Member

@joperezr joperezr left a comment

Choose a reason for hiding this comment

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

Looks like this is ready to go.

@ReubenBond ReubenBond merged commit 1c2f081 into main Feb 14, 2024
3 checks passed
@ReubenBond ReubenBond deleted the preview.3 branch February 14, 2024 20:46
@philliphoff
Copy link

perhaps you know how we might resolve this

@ReubenBond An ugly way would be to set the DaprOptions.DaprPath value to the CLI path found in PATH in the application host when adding Dapr support in the sample. I've never looked at the GH Action before; I didn't realize it dumped it in a random directory. Agree that it could be useful to use what's found in PATH, if anything, before looking at other specific locations.

meneasysoft pushed a commit to meneasysoft/aspire-samples that referenced this pull request Jul 25, 2024
---------

Co-authored-by: Damian Edwards <[email protected]>
Co-authored-by: Reuben Bond <[email protected]>
Co-authored-by: ReubenBond <[email protected]>
Co-authored-by: Reuben Bond <[email protected]>
Co-authored-by: Jose Perez Rodriguez <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants