Skip to content

Extract Aspire.Hosting.Garnet.Tests project#4871

Merged
eerhardt merged 9 commits intomicrosoft:mainfrom
Alirexaa:garnet-tests
Jul 29, 2024
Merged

Extract Aspire.Hosting.Garnet.Tests project#4871
eerhardt merged 9 commits intomicrosoft:mainfrom
Alirexaa:garnet-tests

Conversation

@Alirexaa
Copy link
Copy Markdown
Contributor

@Alirexaa Alirexaa commented Jul 12, 2024 •
edited
Loading

Contributes to #3185
Contributes to #4294

Microsoft Reviewers: Open in CodeFlow

@Alirexaa Alirexaa requested a review from mitchdenny as a code owner July 12, 2024 10:04
@ghost ghost added the area-codeflow for labeling automated codeflow. intentionally a different color! label Jul 12, 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 12, 2024
@Alirexaa
Copy link
Copy Markdown
Contributor Author

Alirexaa commented Jul 12, 2024

@eerhardt, we need to import microsoft/garnet:1.0 to netaspireci.azurecr.io.

@Alirexaa
Copy link
Copy Markdown
Contributor Author

Alirexaa commented Jul 12, 2024

This PR needs more work to do:
Garnet WithPersistence does not work according to #4870.
We need to fix that and then add a functional test for that.

@Alirexaa Alirexaa mentioned this pull request Jul 12, 2024
Closed
16 tasks
@build-analysis build-analysis bot mentioned this pull request Jul 12, 2024
Closed
@eerhardt eerhardt mentioned this pull request Jul 12, 2024
Closed
20 tasks
@eerhardt
Copy link
Copy Markdown
Member

eerhardt commented Jul 15, 2024

@eerhardt, we need to import microsoft/garnet:1.0 to netaspireci.azurecr.io.

We reference microsoft/garnet from ghcr.io and not DockerHub. So we shouldn't need to use our docker hub mirror.

@eerhardt
Copy link
Copy Markdown
Member

eerhardt commented Jul 15, 2024

Can we remove the reference from

https://github.com/dotnet/aspire/blob/f38b6cba86942ad1c45fc04fe7170f0fd4ba7c0b/tests/Aspire.Hosting.Tests/Aspire.Hosting.Tests.csproj#L32

@Alirexaa
Copy link
Copy Markdown
Contributor Author

Alirexaa commented Jul 15, 2024

Can i fix #4870 in this PR?
@eerhardt

@eerhardt
Copy link
Copy Markdown
Member

eerhardt commented Jul 15, 2024

Can i fix #4870 in this PR? @eerhardt

I'd prefer if they were separate. One PR == one change. So make a PR that extracts the tests. And another PR that fixes the issue.

@Alirexaa
Copy link
Copy Markdown
Contributor Author

Alirexaa commented Jul 16, 2024

Can i fix #4870 in this PR? @eerhardt

I'd prefer if they were separate. One PR == one change. So make a PR that extracts the tests. And another PR that fixes the issue.

I will send another PR for that.

@Alirexaa
Copy link
Copy Markdown
Contributor Author

Alirexaa commented Jul 23, 2024

@eerhardt, please take a look, this should be enough at this moment.

@radical radical added area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication testing ☑️ and removed area-codeflow for labeling automated codeflow. intentionally a different color! labels Jul 24, 2024
eerhardt
eerhardt reviewed Jul 29, 2024
View reviewed changes
Copy link
Copy Markdown
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, @Alirexaa.

Can you also add functional tests for WithDataVolume and WithDataBindMount?

@Alirexaa
Copy link
Copy Markdown
Contributor Author

Alirexaa commented Jul 29, 2024

Thanks for the contribution, @Alirexaa.

Can you also add functional tests for WithDataVolume and WithDataBindMount?

I will do, but those tests not pass until #5087

@eerhardt
Copy link
Copy Markdown
Member

eerhardt commented Jul 29, 2024

I will do, but those tests not pass until #5087

oh right. I forgot about that. Sorry.

eerhardt
eerhardt approved these changes Jul 29, 2024
View reviewed changes
Copy link
Copy Markdown
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.

LGTM. Thanks again!

@eerhardt eerhardt merged commit 7eefa05 into microsoft:main Jul 29, 2024
@Alirexaa Alirexaa deleted the garnet-tests branch July 29, 2024 17:18
@github-actions github-actions bot locked and limited conversation to collaborators Aug 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@eerhardt eerhardt eerhardt approved these changes

@mitchdenny mitchdenny Awaiting requested review from mitchdenny mitchdenny is a code owner

@radical radical Awaiting requested review from radical radical is a code owner

Assignees

No one assigned

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 testing ☑️

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Alirexaa @eerhardt @radical