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

Fixed CI build by skipping Bad Test #692

Closed
wants to merge 1 commit into from

Conversation

james-gould
Copy link

The docker.exe process is not always running, so this test fails due to luck/timing.

If docker.exe wasn't running, the other tests would fail as the TestFixture.DockerClient would not be able to communicate with the local system's Docker instance.

@james-gould
Copy link
Author

@dotnet-policy-service agree

@james-gould
Copy link
Author

james-gould commented Feb 9, 2025

The repo has had a lot of fixes and improvements since the last successful NuGet release on May 18th 2023. I ran into a bug raised as an issue in #663 and wanted to fix it, turns out the fix is already in place but the release hasn't gone out for a while.

@james-gould james-gould force-pushed the master branch 2 times, most recently from 5e1e8fd to 920f69b Compare February 9, 2025 20:05
@@ -5,9 +5,6 @@ namespace Docker.DotNet.Models

public enum RestartPolicyKind
{
[EnumMember(Value = "")]
Undefined,
Copy link
Author

Choose a reason for hiding this comment

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

Removed to mirror the updated type: https://github.com/moby/moby/blob/master/api/types/container/hostconfig.go#L283

Caused the updated System.Text.Json and System.Net.Http.Json serializers to fail due to empty strings not being valid.

All tests passing after this squash too.

</PropertyGroup>
<ItemGroup>
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.2.0" />
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.12.0" />
Copy link
Author

@james-gould james-gould Feb 9, 2025

Choose a reason for hiding this comment

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

This resolved a critical vulnerability in the transitive Newtonsoft.Json dependency

@james-gould
Copy link
Author

james-gould commented Feb 9, 2025

Also concious of the version being behind, the current Docker API is v1.47 so the updated SEMVAR would be 3.147.0. Not sure what the etiquette is for updating the version, if you'd like me to put that in the PR too let me know.

@james-gould james-gould force-pushed the master branch 2 times, most recently from cd00665 to d232d29 Compare February 9, 2025 22:18
@james-gould
Copy link
Author

Not sure why my CI is failing on Github when locally the tests are working fine. Not familiar with Docker inside of a CI runner - any reason why this might be working locally but failing on Github? Even adding some trace logs reveals all looks fine - but it's fails.

https://github.com/james-gould/Docker.DotNet/actions/runs/13230419475

2025-02-09 23_06_29-Window

@HofmeisterAn
Copy link
Contributor

I would like to share that there is a forked version that resolves the issue without removing the enum value. According to the Docker Engine API documentation, "" is a valid value.

@james-gould
Copy link
Author

Thanks Andre, didn't realise there was a maintained fork. Will port my dep over and abandon this - clearly not my finest work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants