Skip to content

Remove the test-specific Docker network - #938

Merged
MakisH merged 1 commit into
developfrom
docker-network
Sep 30, 2026
Merged

MakisH merged 1 commit into
developfrom
docker-network

Conversation

@MakisH

@MakisH MakisH commented Sep 30, 2026 •

Copy link
Copy Markdown
Member

Now that multiple GitHub Actions runners run on the same system, the approach of pruning Docker networks (#820) can lead to race conditions. The same applies to when running tests in parallel (#789).

This PR replaces the call to docker network prune (before and after the test) to a docker network ls filtered by working directory, followed by a docker network rm with the respective id. This is executed only after the test, as cleanup.

Testing in https://github.com/precice/tutorials/actions/runs/36738197148 - seems to work: networks are not accumulated.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Network discovery is nonfunctional, removal errors are hidden, and failed tests bypass cleanup.

Review effort: Balanced
Findings: 1 High severity · 2 Medium severity · 1 Low severity

Open (4)
What changed in this PR

Replaces global Docker network pruning with per-system-test network cleanup to avoid parallel-run races.

Changes:

  • Adds targeted Docker network discovery and removal.
  • Runs cleanup after tests and updates the changelog.
File Description
tests/​systemtests/​Systemtest.py Implements targeted network cleanup.
changelog-entries/​820.md Updates the cleanup release note.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread tests/systemtests/Systemtest.py Outdated
Comment thread tests/systemtests/Systemtest.py Outdated
Comment thread tests/systemtests/Systemtest.py Outdated
Comment thread changelog-entries/820.md Outdated
@MakisH
MakisH requested a balanced review from Copilot September 30, 2026 16:27
@MakisH
MakisH requested a balanced review from Copilot and removed request for Copilot September 30, 2026 16:32
@MakisH
MakisH requested a balanced review from Copilot and removed request for Copilot September 30, 2026 16:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The filter cannot find Compose networks, and destructor-based cleanup is deferred and unreliable.

Review effort: Balanced
Findings: 2 High severity · 1 Medium severity · 1 Low severity

Open (4)
Resolved since last review (4)

Comment thread tests/systemtests/Systemtest.py Outdated
Comment thread tests/systemtests/Systemtest.py Outdated
Comment thread tests/systemtests/Systemtest.py Outdated
Comment thread changelog-entries/820.md Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Concurrent runners can share a project identifier, allowing one test to remove another test’s network.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)
Resolved since last review (4)

Comment thread tests/systemtests/Systemtest.py
Comment thread tests/systemtests/Systemtest.py Outdated
@MakisH
MakisH marked this pull request as ready for review September 30, 2026 17:01
@MakisH
MakisH merged commit 28c3223 into develop Sep 30, 2026
1 check passed
@MakisH
MakisH deleted the docker-network branch September 30, 2026 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants