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 53 commits into
base: main
Choose a base branch
from
Draft

Trying out gha to run tests #7073

wants to merge 53 commits into from

Conversation

davidfowl
Copy link
Member

@davidfowl davidfowl commented Jan 12, 2025

Trying this out

Comment on lines +309 to +318
// Need to grant read access to the config file on unix like systems.
if (!OperatingSystem.IsWindows())
{
// 777
var permissions = UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute |
UnixFileMode.GroupRead | UnixFileMode.GroupWrite | UnixFileMode.GroupExecute |
UnixFileMode.OtherRead | UnixFileMode.OtherWrite | UnixFileMode.OtherExecute;

File.SetUnixFileMode(serverFileMount.Source!, permissions);
}
Copy link
Member

Choose a reason for hiding this comment

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

This can be done as part of the call to CreateDirectory above:

    public static class Directory
    {
        [UnsupportedOSPlatform("windows")]
        public static DirectoryInfo! CreateDirectory(string! path, UnixFileMode unixCreateMode);
    }

@@ -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?

filter: "WithDataShouldPersistStateBetweenUsages"
- project: tests/Aspire.Hosting.PostgreSQL.Tests/Aspire.Hosting.PostgreSQL.Tests.csproj
name: PostgreSQL
filter: "Aspire.Hosting"
Copy link
Member

Choose a reason for hiding this comment

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

Are these filters just for now? I wouldn't expect us to use them when we did this for real.

@@ -61,7 +61,7 @@ public static TestDistributedApplicationBuilder Create(Action<DistributedApplica
return new TestDistributedApplicationBuilder(configureOptions, testOutputHelper);
}

public static TestDistributedApplicationBuilder CreateWithTestContainerRegistry(ITestOutputHelper testOutputHelper) =>
public static TestDistributedApplicationBuilder CreateWithTestContainerRegistry(ITestOutputHelper? testOutputHelper = null) =>
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be reverted. We always want an ITestOutputHelper to be passed so we get log output correctly.

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 was debugging 😄

Comment on lines +575 to +581
if (!OperatingSystem.IsWindows())
{
var permissions = UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute |
UnixFileMode.GroupRead | UnixFileMode.GroupWrite | UnixFileMode.GroupExecute |
UnixFileMode.OtherRead | UnixFileMode.OtherWrite | UnixFileMode.OtherExecute;

File.SetUnixFileMode(bindMountPath, permissions);
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't be doing this in the test - this seems like a product bug.

timeout-minutes: 60
strategy:
fail-fast: false
matrix:
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that for each test assembly we will get a separate job/leg in CI?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct

@danmoseley danmoseley added the area-engineering-systems infrastructure helix infra engineering repo stuff label Jan 13, 2025
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.

3 participants