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.

Sign up for GitHub

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

Jump to bottom

Hosting.Keycloak: Fix importDirectory param validation in WithRealmImport #5220

Merged
merged 13 commits into from
Aug 14, 2024
Merged

Hosting.Keycloak: Fix importDirectory param validation in WithRealmImport #5220

merged 13 commits into from
Aug 14, 2024

Conversation

radical
Copy link
Member

@radical radical commented Aug 7, 2024 •
edited
Loading

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

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Aug 7, 2024
@radical radical mentioned this pull request Aug 7, 2024
Closed
@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
@radical
Hosting.KeyCloak: Remove incorrect param check on WithRealmImport
972f9a9 
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.

aspire/playground/keycloak/Keycloak.ApiService/Program.cs

Lines 28 to 40 in 972f9a9

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
@radical
[tests] Add Keycloak playground app to tests
ac3dbc4
@radical
Add test for WithRealmImport, and use the full path for directory-exi…
ae522b4 
…sts check
@radical
Merge remote-tracking branch 'origin/main' into fix-hosting-testing-cwd
ad0f520 
# 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.

aspire/src/Aspire.Hosting/ApplicationModel/ContainerMountAnnotation.cs

Lines 18 to 19 in ac3dbc4

/// <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
julioct
julioct approved these changes Aug 9, 2024
View reviewed changes
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
Open
35 tasks
@radical @eerhardt
Update KeycloakResourceBuilderExtensions.cs
eda8c99 
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
@radical
Copy link
Member Author

radical commented Aug 12, 2024

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@build-analysis build-analysis bot mentioned this pull request Aug 12, 2024
Open
@radical
Merge remote-tracking branch 'origin/main' into fix-hosting-testing-cwd
7dd3695 
# Conflicts:
#	tests/Aspire.Playground.Tests/AppHostTests.cs
#	tests/Aspire.Playground.Tests/Aspire.Playground.Tests.csproj
@build-analysis build-analysis bot mentioned this pull request Aug 13, 2024
Open
3 tasks
@radical @eerhardt
Update src/Aspire.Hosting.Keycloak/KeycloakResourceBuilderExtensions.cs
ac2d9fd 
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
@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
11 checks passed
@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.
Reviewers

@julioct julioct

@eerhardt eerhardt

@sebastienros sebastienros

@joperezr joperezr

Assignees
No one assigned
Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@radical @julioct @eerhardt @joperezr