Skip to content

Conversation

@radical
Copy link
Member

@radical radical commented Aug 7, 2024

Description

The import path is expected to be relative to the AppHost path, but here
it was being used as-is - relative to the current working directory.
Instead, expand the path relative to the AppHost path and use that for Directory.Exists check.

This manifested as a failure to import the file when running the app
with Aspire.Hosting.Testing which would start the apphost with
cwd!=apphost .

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-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Aug 7, 2024
@radical radical force-pushed the fix-hosting-testing-cwd branch from 81f9b6b to 88d4681 Compare August 9, 2024 00:06
@radical radical changed the title Aspire.Hosting.Testing: Update CWD when starting an apphost Hosting.Keycloak: Remove incorrect param check in WithRealmImport Aug 9, 2024
The import path is expected to be relative to the AppHost path, but here
it was being used as-is - relative to the current working directory.
Instead, remove the check and let the downstream `WithBindMount` method
use the path correctly.

This manifested as a failure to import the file when running the app
with `Aspire.Hosting.Testing` which would start the apphost with
cwd!=apphost .
@radical radical marked this pull request as ready for review August 9, 2024 00:09
@radical radical force-pushed the fix-hosting-testing-cwd branch from 88d4681 to 972f9a9 Compare August 9, 2024 00:11
@radical radical requested a review from sebastienros August 9, 2024 00:11
@radical
Copy link
Member Author

radical commented Aug 9, 2024

cc @julioct

@radical radical requested a review from joperezr August 9, 2024 00:35
@radical
Copy link
Member Author

radical commented Aug 9, 2024

We can drop the changes in TestingAppHost1, but then we need to add a test somewhere else. I did try adding Keycloak playground app to the tests for which /alive, and /health work fine but /weatherforecast fails with Endpoint 'http://localhost:52119/weatherforecast' for resource 'apiservice' in app 'Keycloak.AppHost' returned status code Unauthorized.

app.MapGet("/weatherforecast", () =>
{
var forecast = Enumerable.Range(1, 5).Select(index =>
new WeatherForecast
(
DateOnly.FromDateTime(DateTime.Now.AddDays(index)),
Random.Shared.Next(-20, 55),
summaries[Random.Shared.Next(summaries.Length)]
))
.ToArray();
return forecast;
})
.RequireAuthorization();

radical added 3 commits August 9, 2024 17:07
# Conflicts:
#	tests/Aspire.Playground.Tests/AppHostTests.cs
#	tests/Aspire.Playground.Tests/Aspire.Playground.Tests.csproj
@radical
Copy link
Member Author

radical commented Aug 9, 2024

Looks like WithBindMount does not expect the path to exist. And ContainerMountAnnotation even allows a null source path.

/// <param name="source">The source path if a bind mount or name if a volume. Can be <c>null</c> if the mount is an anonymous volume.</param>
/// <param name="target">The target path of the mount.</param>

I'll restore the check, and expand the path using the apphost path.

@radical radical changed the title Hosting.Keycloak: Remove incorrect param check in WithRealmImport Hosting.Keycloak: Fix importDirectory param validation in WithRealmImport Aug 9, 2024
Copy link
Contributor

@julioct julioct left a comment

Choose a reason for hiding this comment

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

LGTM

@radical radical mentioned this pull request Aug 12, 2024
35 tasks
@radical
Copy link
Member Author

radical commented Aug 12, 2024

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

# Conflicts:
#	tests/Aspire.Playground.Tests/AppHostTests.cs
#	tests/Aspire.Playground.Tests/Aspire.Playground.Tests.csproj
@radical
Copy link
Member Author

radical commented Aug 13, 2024

Merge after #5276 .

@radical radical enabled auto-merge (squash) August 14, 2024 01:21
@radical radical merged commit 1e98134 into dotnet:main Aug 14, 2024
@radical radical deleted the fix-hosting-testing-cwd branch August 14, 2024 01:39
@github-actions github-actions bot locked and limited conversation to collaborators Sep 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants