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

close #2636: added json schemas for Aspire.Hosting.AppHost and Aspire.Hosting.Azure #4912

Merged

Conversation

vladimir-shirmanov
Copy link
Contributor

@vladimir-shirmanov vladimir-shirmanov commented Jul 15, 2024

Description
fixed an issues with intellisense inside appsettings.json when using Aspire.Hosting.AppHost or Aspire.Hosting.Azure

Changes:

  • added AspireAppHostConfiguration.json file and included it into Aspire.Host.AppHosting nuget package
  • added src/Aspire.Hosting.Azure/AspireAzureConfigurationSchema.json and included it into Aspire.Host.Azure package

This will close #2636

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 Jul 15, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 15, 2024
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, @vladimir-shirmanov!

@@ -16,7 +16,7 @@
</PropertyGroup>

<ItemGroup>
<None Include="**/*.props;**/*.targets" Pack="true" PackagePath="%(RecursiveDir)%(Filename)%(Extension)" />
<None Include="**/*.props;**/*.targets;*.json" Pack="true" PackagePath="%(RecursiveDir)%(Filename)%(Extension)" />
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<None Include="**/*.props;**/*.targets;*.json" Pack="true" PackagePath="%(RecursiveDir)%(Filename)%(Extension)" />
<None Include="**/*.props;**/*.targets" Pack="true" PackagePath="%(RecursiveDir)%(Filename)%(Extension)" />
<None Include="AspireAppHostConfiguration.json" Pack="true" />

Maybe we should just pack the one file we want, that way we don't accidently start including other .json files we don't intend to put in the nupkg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, fixed it

<Project>
<ItemGroup>
<JsonSchemaSegment Include="$(MSBuildThisFileDirectory)..\AspireAzureConfigurationSchema.json"
FilePathPattern="appsettings\..*json" />
Copy link
Member

Choose a reason for hiding this comment

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

A lot of times these properties go into "User Secrets", which is a secrets.json file. This is a Regex, so I think we should also add secrets.json to it. @alexgav - did we ever hook up the FilePathPattern correctly in VS?

Suggested change
FilePathPattern="appsettings\..*json" />
FilePathPattern="appsettings\..*json|secrets.json" />

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to add it, but it didn't work for me. So I skipped for now

@@ -0,0 +1,6 @@
<Project>
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 we only need a .targets file in buildTransitive. We shouldn't need the other 2 targets files. Just move this logic in there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, removed all unnecessary ones

@@ -19,11 +19,15 @@
<Compile Include="..\Aspire.Hosting\Dcp\Process\ProcessUtil.cs" Link="Provisioning\Utils\ProcessUtil.cs" />
<Compile Include="..\Shared\Cosmos\CosmosConstants.cs" Link="Shared\CosmosConstants.cs" />
</ItemGroup>

Copy link
Member

Choose a reason for hiding this comment

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

(nit) this can be reverted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah sorry, removed it as well

@vladimir-shirmanov
Copy link
Contributor Author

vladimir-shirmanov commented Jul 17, 2024

@eerhardt thank you for the feedback.
I made the changes you asked for

Add some more descriptions and tweak wording.
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Nice work @vladimir-shirmanov! Thank you for this contribution. This looks good to me. I'm setting it to 'auto-merge' when CI passes.

@eerhardt eerhardt enabled auto-merge (squash) August 26, 2024 20:37
@eerhardt eerhardt merged commit 4251f91 into dotnet:main Aug 26, 2024
11 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 26, 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 community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[WebToolsE2E][Aspire] There is no intellisense of 'Parameters' in the appsettings.json file.
2 participants