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

Extract Aspire.Hosting.SqlServer.Tests project #5056

Merged
merged 35 commits into from
Aug 9, 2024
Merged

Extract Aspire.Hosting.SqlServer.Tests project #5056

merged 35 commits into from
Aug 9, 2024

Conversation

Alirexaa
Copy link
Contributor

@Alirexaa Alirexaa commented Jul 24, 2024 •
edited by dotnet-policy-service bot
Loading

Contributes to #3185
Contributes to #4294

Microsoft Reviewers: Open in CodeFlow

@Alirexaa Alirexaa requested a review from mitchdenny as a code owner July 24, 2024 18:48
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-codeflow for labeling automated codeflow. intentionally a different color! label Jul 24, 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 24, 2024
@Alirexaa
Copy link
Contributor Author

Alirexaa commented Jul 24, 2024 •
edited
Loading

WithDataShouldPersistStateBetweenUsages(useVolume: true) work on my local but WithDataShouldPersistStateBetweenUsages(useVolume: false) failed because of #5055.
For now, I disabled this test to discuss it.

@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
@sebastienros
Copy link
Member

sebastienros commented Jul 29, 2024 •
edited
Loading

Something I was asked to do, and maybe you can take what I did as an example, if to also use the EF component in this functional test to extend the coverage.

@eerhardt eerhardt mentioned this pull request Jul 29, 2024
Closed
20 tasks
@sebastienros
Copy link
Member

Build failing on an unrelated flaky test. @eerhardt can you review?

eerhardt
eerhardt reviewed Aug 5, 2024
View reviewed changes
src/Aspire.Hosting.SqlServer/SqlServerBuilderExtensions.cs
@@ -71,5 +71,21 @@ public static IResourceBuilder<SqlServerServerResource> WithDataVolume(this IRes
/// <param name="isReadOnly">A flag that indicates if this is a read-only mount.</param>
/// <returns>The <see cref="IResourceBuilder{T}"/>.</returns>
public static IResourceBuilder<SqlServerServerResource> WithDataBindMount(this IResourceBuilder<SqlServerServerResource> builder, string source, bool isReadOnly = false)
=> builder.WithBindMount(source, "/var/opt/mssql", isReadOnly);
{
// c.f. https://learn.microsoft.com/sql/linux/sql-server-linux-docker-container-configure?view=sql-server-ver15&pivots=cs1-bash#mount-a-host-directory-as-data-volume
Copy link
Member

Choose a reason for hiding this comment

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

Why is this done only for WithDataBindMount and not for WithDataVolume?

Copy link
Member

Choose a reason for hiding this comment

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

Binding specific folders only applies to folder mounts, not volumes. Volumes just work fine locally without changes, and the documentation only uses it for folders mounts too.

Copy link
Member

Choose a reason for hiding this comment

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

@rwestMSFT - I see you wrote the documentation being referenced here in MicrosoftDocs/sql-docs@51dddc6. Is there a reason we need to split all 3 directories when binding to a host directory, but when using a docker volume you just need a single volume? Why doesn't it work to map the single top-level /var/opt/mssql directory to the host machine?

cc @JerryNixon

Copy link

@rwestMSFT rwestMSFT Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@eerhardt My name appears on thousands of articles. Please refer to the author in metadata when reviewing SQL Docs content.

Copy link
Member

Choose a reason for hiding this comment

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

@amitkh-msft - Looks like you are the "author" of the article referenced above. Any chance you can answer the above question?

Copy link

Choose a reason for hiding this comment

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

Ack. Sure, please give me some time. I will look at this and try to answer the question.

Copy link
Member

Choose a reason for hiding this comment

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

Any thoughts @amitkh-msft? Why can't we just bind mount to the root directory instead of the 3 separate ones?

tests/testproject/TestProject.IntegrationServiceA/TestProject.IntegrationServiceA.csproj Outdated Show resolved Hide resolved
tests/Aspire.Hosting.SqlServer.Tests/TestDbContext.cs Outdated Show resolved Hide resolved
tests/Aspire.Hosting.SqlServer.Tests/SqlServerFunctionalTests.cs Outdated Show resolved Hide resolved
tests/Aspire.Hosting.SqlServer.Tests/SqlServerFunctionalTests.cs Outdated Show resolved Hide resolved
tests/Aspire.Hosting.SqlServer.Tests/SqlServerFunctionalTests.cs Outdated Show resolved Hide resolved
tests/Aspire.Hosting.SqlServer.Tests/Aspire.Hosting.SqlServer.Tests.csproj Outdated Show resolved Hide resolved
This was referenced Aug 6, 2024
Open
Open
eerhardt
eerhardt approved these changes Aug 7, 2024
View reviewed changes
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.

I think my only real concern is the product change to WithDataBindMount. I don't understand why the 3 directories are needed, as opposed to just 1 root level directory.

But we can follow up later around this, if needed.

tests/Aspire.Hosting.SqlServer.Tests/SqlServerFunctionalTests.cs Outdated Show resolved Hide resolved
@sebastienros sebastienros merged commit 0d8b294 into dotnet:main Aug 9, 2024
11 checks passed
@Alirexaa Alirexaa deleted the sqlserver-tests branch August 9, 2024 07:54
@github-actions github-actions bot locked and limited conversation to collaborators Sep 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@amitkh-msft amitkh-msft amitkh-msft left review comments

@rwestMSFT rwestMSFT rwestMSFT left review comments

@eerhardt eerhardt eerhardt approved these changes

@sebastienros sebastienros sebastienros left review comments

@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.
6 participants
@Alirexaa @sebastienros @eerhardt @rwestMSFT @amitkh-msft @radical