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 Garnet Resource #3839

Merged
merged 75 commits into from
May 7, 2024
Merged

Add Garnet Resource #3839

merged 75 commits into from
May 7, 2024

Conversation

Zombach
Copy link
Contributor

@Zombach Zombach commented Apr 19, 2024

Implemented support request from #3428

Microsoft Reviewers: Open in CodeFlow

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-integrations Issues pertaining to Aspire Integrations packages label Apr 19, 2024
@Zombach Zombach changed the title Add Garnet Powered by Redis Add Garnet Component Apr 19, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 19, 2024
@Zombach
Copy link
Contributor Author

Zombach commented Apr 19, 2024

@dotnet-policy-service agree

@davidfowl davidfowl added area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication and removed area-integrations Issues pertaining to Aspire Integrations packages labels Apr 20, 2024
@davidfowl davidfowl changed the title Add Garnet Component Add Garnet Resource Apr 20, 2024
recommendation from davidfowl

Co-authored-by: David Fowler <davidfowl@gmail.com>
@Zombach Zombach requested a review from davidfowl April 20, 2024 08:20
@mitchdenny
Copy link
Member

Can you also integrate this with the testproject? Take a look at Redis for how it is done - we'd just want this to sit along side Redis as if it were something completely different rather than do anything clever and reuse the code between them (obviously we'll use the same client).

@Zombach
Copy link
Contributor Author

Zombach commented May 3, 2024

Hello @mitchdenny. Could you watch it again?

/// Adds a Garnet container to the application model.
/// </summary>
/// <example>
/// <remarks>Use in AspireHost</remarks>
Copy link
Member

Choose a reason for hiding this comment

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

I don't think <remarks /> is a child element of <example /> What is AspireHost in this context -- or do you mean application host?

Copy link
Member

Choose a reason for hiding this comment

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

OK so I've look through the XML doc comments. It's great that you've got some examples in there. I think we just need the XML doc comments applied consistently. Example:

/// <summary>A short summary of what the type/member is.</summary>
/// <remarks>
/// <para>Some longer form content with background information if
/// it would provide value.</para>
/// <para>You can use the para element to break the text up. This
/// text would not necessarily have code but would use plenty
/// of references.</para>
/// <example>
/// A description of the example and what it shows.
/// <code>
/// The actual code.
/// </code>
/// </example>
/// <example>
/// Another example with a description.
/// <code>
/// The actual code.
/// </code>
/// </example>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like I fixed everything

@mitchdenny
Copy link
Member

Hello @mitchdenny. Could you watch it again?

I've read through the only thing that I think needs a touch up is the doc comments there are a few inconsistencies and (I think) invalid XML patterns. I provided an example. Once that is done we can get this merged in for 8.1!

@mitchdenny mitchdenny added the needs-author-action An issue or pull request that requires more info or actions from the author. label May 6, 2024
@Zombach
Copy link
Contributor Author

Zombach commented May 6, 2024

Hello @mitchdenny. Could you watch it again?

I've read through the only thing that I think needs a touch up is the doc comments there are a few inconsistencies and (I think) invalid XML patterns. I provided an example. Once that is done we can get this merged in for 8.1!

I'll try to do it in the evening.
Thank you

@dotnet-policy-service dotnet-policy-service bot removed the needs-author-action An issue or pull request that requires more info or actions from the author. label May 6, 2024
@mitchdenny mitchdenny enabled auto-merge (squash) May 7, 2024 06:46
@mitchdenny
Copy link
Member

Thanks for the contribution and your patience working through all the feedback!

@mitchdenny mitchdenny merged commit 9fab2f6 into dotnet:main May 7, 2024
8 checks passed
@Zombach
Copy link
Contributor Author

Zombach commented May 7, 2024

Thanks for the contribution and your patience working through all the feedback!

Thank you too. It was informative and interesting

@github-actions github-actions bot locked and limited conversation to collaborators Jun 6, 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.

5 participants