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

Add tags to hosting packages for easier searching #1462

Merged

Conversation

kiapanahi
Copy link
Contributor

@kiapanahi kiapanahi commented Dec 19, 2023

Related to #1314

This PR moves all Aspire.Hosting.* project to /src/Hosting/ and adds a directory-specific Directory.Build.props to said directory to manage common and specific hosting-related tags.

It also adds relative <PackageTags>...<PackageTags/> to all Aspire.Hosting.* projects.

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

This PR moves all Aspire.Hosting.* project to /src/Hosting/ and adds a directory-specific Directory.Build.props to said directory to manage common and specific hosting-related tags.

This sort of huge move isn't great without coordination with everyone working on the project. This isn't something we would take as it is a very disruptive change that affects all existing changes anyone is making.

Can you make a smaller change that doesn't require moving all of the existing source code around?

@kiapanahi
Copy link
Contributor Author

Can you make a smaller change that doesn't require moving all of the existing source code around?

This would require "manually" adding project tags to all hosting-related projects due to the missing localized Directory.Build.props.

The downside would be "some" manual overseeing and orchestration. On the upside, the changes would be dramatically less.
Would that be acceptable to the team?

@davidfowl
Copy link
Member

The downside would be "some" manual overseeing and orchestration. On the upside, the changes would be dramatically less.
Would that be acceptable to the team?

Yes

@kiapanahi kiapanahi force-pushed the issue_1314/hosting-packages-tags branch from ec83d4e to 469aaf1 Compare December 19, 2023 09:28
@kiapanahi
Copy link
Contributor Author

Done :)

@davidfowl
Copy link
Member

5 files instead of 209 😄

@sebastienros
Copy link
Member

@kiapanahi you should rebased your branch on main, the current diff includes too many other unrelated changes.

@kiapanahi kiapanahi force-pushed the issue_1314/hosting-packages-tags branch from 47ddad0 to 64ebada Compare December 28, 2023 20:20
@kiapanahi
Copy link
Contributor Author

kiapanahi commented Dec 28, 2023

@kiapanahi you should rebased your branch on main, the current diff includes too many other unrelated changes.

Had done that on my other machine but somehow it was not pushed (or I forgot to update my forked main branch beforehand maybe).

Just updated the branch again; now it only includes the 5 related changes

@sebastienros
Copy link
Member

Much better, thanks.

Now that I see the changes, I wonder how it actually makes the search better? The tags are only the list of word that the package names are made of, for instance aspire hosting azure provisioning for Aspire.Hosting.Azure.Provisioning. At that point I wondered if NuGet was not already clever enough to tokenize the package names using the dot, so I did some searches with Aspire, or Aspire Azure, Aspire Dapr, and got exactly the packages you would expect. And these are the exact keywords you are suggesting in this PR. At that point I am not sure how it makes the search easier.

I tried on nuget.org, maybe Visual Studio uses a different API for the search.

@kiapanahi
Copy link
Contributor Author

I tried on nuget.org, maybe Visual Studio uses a different API for the search.

Fine point. I don't see why the APIs should follow a different search path than the web application front end. I haven't tested myself either.

The main reason for this PR was to address @timheuer's concert for consistency rather than search optimization (link to the issue). I admit the titles (both the issue and this PR) could be seen as a tad misleading though. If the consistency of package names is not a relatively valid concern in the hosting part of Aspire packages then this work would probably loose its main purpose.

Back to your point on NuGet being clever enough, I naively didn't do the search myself and just assumed that it wasn't that clever.

@sebastienros
Copy link
Member

One advantage I see with tags is that you can create formal groups, instead of relying on word similarity matches.

I had not paid attention to the issue you are referencing, sorry.

@sebastienros sebastienros merged commit 1aa2276 into dotnet:main Dec 29, 2023
8 checks passed
@kiapanahi kiapanahi deleted the issue_1314/hosting-packages-tags branch December 31, 2023 10:51
@github-actions github-actions bot locked and limited conversation to collaborators Apr 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.

3 participants