fix: service network not attached + complete service adapter conversions#525
fix: service network not attached + complete service adapter conversions#525
Conversation
Fix convertToSwarmSpec reading networks only from ServiceSpec.Networks, ignoring TaskTemplate.Networks where buildService() actually writes them. This caused job-service-run network attachments to be silently dropped (closes #524). Fix convertFromSwarmService missing conversions for Mounts, RestartPolicy, Resources, Networks, and Mode — all were silently lost when inspecting services. Add comprehensive round-trip tests (domain→swarm→domain) that verify every field survives the conversion cycle. All 8 round-trip tests were written before the fixes and confirmed failing (TDD). Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug where Docker Swarm service network attachments were silently dropped due to a mismatch in how network configurations were read and written within the domain layer. It also significantly enhances the fromSwarm conversion logic to ensure a complete and accurate round-trip conversion of various service specifications, including mounts, restart policies, resource limits, and service modes, thereby improving the robustness and reliability of service management. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
✅ Mutation Testing ResultsMutation Score: 100.00% (threshold: 60%)
What is mutation testing?Mutation testing measures test quality by introducing small changes (mutations) to the code and checking if tests detect them. A higher score means better test effectiveness.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request addresses several conversion bugs in core/adapters/docker/service.go. The convertToSwarmSpec function was updated to correctly aggregate network configurations from both service and task template levels. The convertFromSwarmService function was significantly enhanced to properly convert various service properties, including container mounts, restart policies, resource requirements (limits and reservations), network attachments, and service mode (replicated or global), from Docker Swarm's swarm.Service structure back to the internal domain.Service representation. These fixes are validated by new, comprehensive round-trip tests in service_roundtrip_test.go and a new test in runservice_network_test.go for network wiring. The review comments suggest optimizing slice allocations in convertFromSwarmService for Mounts and Networks by pre-allocating them, and updating outdated "BUG" comments and assertion messages in the new service_roundtrip_test.go file to reflect the resolved issues.
There was a problem hiding this comment.
Pull request overview
Fixes the Swarm service network attachment regression for job-service-run by ensuring the Docker adapter correctly converts network attachments from the domain model to the Swarm SDK model, and expands convertFromSwarmService() to round-trip additional service fields.
Changes:
- Fix
convertToSwarmSpec()to include networks provided via bothServiceSpec.NetworksandTaskTemplate.Networks. - Implement missing
convertFromSwarmService()conversions (mounts, restart policy, resources, networks, service mode). - Add unit tests covering round-trip conversions and a core-level test asserting
RunServiceJob.Networkis wired into the service spec.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| core/adapters/docker/service.go | Fixes network conversion path and adds missing Swarm→domain conversions for service specs. |
| core/adapters/docker/service_roundtrip_test.go | Adds round-trip coverage to validate the adapter conversions end-to-end. |
| core/runservice_network_test.go | Adds a focused regression test to ensure RunServiceJob.Network is preserved into the created service spec. |
Comments suppressed due to low confidence (1)
core/adapters/docker/service.go:249
- convertToSwarmSpec currently merges ServiceSpec.Networks and TaskTemplate.Networks by simple concatenation. If both are set, the same network can be emitted twice, which may cause Docker to reject the spec or attach twice. Consider de-duplicating by Target (and possibly merging/unioning Aliases) and avoid potential slice aliasing by copying before append.
// Convert networks from both ServiceSpec and TaskTemplate levels.
// buildService() writes to TaskTemplate.Networks; both locations are valid.
allNetworks := append(spec.Networks, spec.TaskTemplate.Networks...)
for _, n := range allNetworks {
swarmSpec.TaskTemplate.Networks = append(swarmSpec.TaskTemplate.Networks, swarm.NetworkAttachmentConfig{
Target: n.Target,
Aliases: n.Aliases,
})
}
You can also share your feedback on Copilot code review. Take the survey.
Fix gocritic appendAssign lint by using separate loops for network sources. Fix gci import ordering. Reword test comments/assertions to describe expected behavior rather than fixed bugs. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
The project gci config uses prefix(github.com/netresearch/ofelia) as a separate section, requiring blank line between external and project imports. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
Add TestServiceSpec_RoundTrip_NilFields verifying that nil RestartPolicy, Resources, Networks, Mounts, and Mode all remain nil/empty after round-trip. Add TestServiceSpec_RoundTrip_RestartConditionNone testing the exact values buildService() uses (Condition=none, MaxAttempts=1, nil Delay/Window). Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
Add toSwarm and fromSwarm conversion for three domain types that existed in the model but were silently dropped during conversion: - TaskSpec.Placement (Constraints + Preferences with SpreadOver) - TaskSpec.LogDriver (Name + Options) - ServiceSpec.EndpointSpec (Mode + Ports with all PortConfig fields) Add 3 round-trip tests verifying full domain→swarm→domain fidelity. Total round-trip tests: 13. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
Split convertToSwarmSpec and convertFromSwarmService into smaller functions (convertTaskTemplateToSwarm, convertTaskTemplateFromSwarm) to bring cyclomatic complexity below the gocyclo threshold of 15. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
CybotTM
changed the title
Distinguish the network bug fix and missing fromSwarm conversions (Fixed) from the new Placement/LogDriver/EndpointSpec support (Added). Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
|
🚀 Released in v0.21.4 Thank you for your contribution! 🙏 This is now available in the latest release. Please test and verify everything works as expected in your environment. If you encounter any issues, please open a new issue. |
Summary
Fixes #524 —
job-service-runnetwork attachment silently dropped, service never joins the specified network.Root cause:
convertToSwarmSpec()read networks fromServiceSpec.Networks, butbuildService()writes toTaskTemplate.Networks. The domain/adapter refactor in 9cefcdf (2025-11-26) introduced this mismatch.Bug fixes
convertToSwarmSpecnetwork mapping: read networks from bothServiceSpec.NetworksandTaskTemplate.NetworksconvertFromSwarmServicemissing conversions: Mounts, RestartPolicy, Resources, Networks, Mode were silently lost when inspecting servicesNew conversions (previously dead domain types)
Placement, LogDriver, and EndpointSpec existed in the domain model but had no conversion code in either direction — silently dropped when set. Now fully wired:
TaskSpec.Placement(Constraints + Preferences with SpreadOver)TaskSpec.LogDriver(Name + Options)ServiceSpec.EndpointSpec(Mode + Ports with all PortConfig fields)Refactoring
Extracted
convertTaskTemplateToSwarmandconvertTaskTemplateFromSwarmhelpers to keep cyclomatic complexity below the gocyclo threshold.Tests — 13 round-trip + 1 integration (TDD)
Plus 1 integration test verifying
buildService()→ domain spec has network.Test plan