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

Trying out gha to run tests #7073

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

Trying out gha to run tests #7073

wants to merge 11 commits into from

Conversation

davidfowl
Copy link
Member

@davidfowl davidfowl commented Jan 12, 2025

Run integration tests on github actions as individual jobs for maximum concurrency.

This pull request introduces a variety of changes aimed at enhancing the functionality and robustness of the Aspire Hosting project. The most significant updates include the addition of integration tests, the introduction of Unix file mode settings for bind mounts, and various improvements to the PostgreSQL and Redis components.

Fixes #4599

@@ -304,6 +306,17 @@ public static IResourceBuilder<PostgresServerResource> WithPgWeb(this IResourceB
Directory.CreateDirectory(serverFileMount.Source!);
}

// Need to grant read access to the config file on unix like systems.
Copy link
Member

Choose a reason for hiding this comment

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

Why wasn't this needed before?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure how our machines or docker is setup.

@danegsta ideas?

Copy link
Member

Choose a reason for hiding this comment

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

The need for custom file permissions on Mac/Linux really depends on the specific container. If the container is running as root, it’s not really necessary, but if a container is running as a custom user, then we have to jump through hoops to make sure that user will be able to read the mounted file. So if the pgweb image started using a specific user instead of root, that’d necessitate the change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Its strange we dont need this on our other machine though right? I had to add it here because I saw permission errors

Copy link
Member Author

Choose a reason for hiding this comment

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

I know this is per container, but I wonder if we can as part of creating a BindMount if we should allow setting these flags..

Copy link
Member Author

Choose a reason for hiding this comment

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

Take a look at the new changes I made WRT bind mounts in the latest commits. I made it first class on bind mount and calls to WithBindMount can opt into what per resource.

I think we solve this on an integration by integration basis depending on what minimum permissions that bind mount needs based on the interaction with the container. I still don't know why I had to make these changes in this pr to make it work though.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@danmoseley danmoseley added the area-engineering-systems infrastructure helix infra engineering repo stuff label Jan 13, 2025

- name: Build projects
run: |
dotnet build ${{ matrix.project }} -c Release /p:CI=false --no-restore
Copy link
Member

Choose a reason for hiding this comment

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

What is /p:CI=false for? Also, as a NIT, I'm assuming we could theoretically collapse restore, build and test phase into a single command as I believe dotnet test should do all of that.

Copy link
Member

Choose a reason for hiding this comment

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

Might also be good to remove the -c Release as Debug will help having more actionable info in callstack in case test fails.

Copy link
Member Author

Choose a reason for hiding this comment

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

What is /p:CI=false for? Also, as a NIT, I'm assuming we could theoretically collapse restore, build and test phase into a single command as I believe dotnet test should do all of that.

I get this #5956

@davidfowl davidfowl force-pushed the davidfowl/gha-experiment branch from 8cd4272 to 8bae5d6 Compare January 16, 2025 08:45
Comment on lines +1500 to +1501
if (mount.Type == ContainerMountType.BindMount &&
mount.UnixFileMode is not null &&
Copy link
Member

Choose a reason for hiding this comment

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

Evaluate the mount with pattern matching.

Suggested change
if (mount.Type == ContainerMountType.BindMount &&
mount.UnixFileMode is not null &&
if (mount is { Type: ContainerMountType.BindMount, UnixFileMode: not null } &&

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-engineering-systems infrastructure helix infra engineering repo stuff
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[tests] Failing test on CI - DistributedApplicationTests.ProxylessAndProxiedEndpointBothWorkOnSameResource
6 participants