Skip to content

Conversation

@nvollmar
Copy link
Contributor

@nvollmar nvollmar commented Nov 1, 2025

Change summary

Simplify container run arguments to reduce chance for unintended inconsistencies between host and container networking

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes)
  • Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
  • Other (please describe):

Related Task(s)

Related PR(s)

How to test / Smoketest result

DEBUG - Running Testcase: /usr/libexec/vyos/tests/smoke/cli/test_container.py
DEBUG - test_api_socket (__main__.TestContainer.test_api_socket) ... ok
DEBUG - test_basic (__main__.TestContainer.test_basic) ... ok
DEBUG - test_cpu_limit (__main__.TestContainer.test_cpu_limit) ... ok
DEBUG - test_dual_stack_network (__main__.TestContainer.test_dual_stack_network) ... ok
DEBUG - test_healthcheck (__main__.TestContainer.test_healthcheck) ... ok
DEBUG - test_ipv4_network (__main__.TestContainer.test_ipv4_network) ... ok
DEBUG - test_ipv6_network (__main__.TestContainer.test_ipv6_network) ... ok
DEBUG - test_name_server (__main__.TestContainer.test_name_server) ... ok
DEBUG - test_network_mtu (__main__.TestContainer.test_network_mtu) ... ok
DEBUG - test_network_types (__main__.TestContainer.test_network_types) ... ok
DEBUG - test_no_name_server (__main__.TestContainer.test_no_name_server) ... ok
DEBUG - test_uid_gid (__main__.TestContainer.test_uid_gid) ... ok
DEBUG - test_user_defined_mac (__main__.TestContainer.test_user_defined_mac) ... ok

Checklist:

  • I have read the CONTRIBUTING document
  • I have linked this PR to one or more Phabricator Task(s)
  • I have run the components SMOKETESTS if applicable
  • My commit headlines contain a valid Task id
  • My change requires a change to the documentation
  • I have updated the documentation accordingly

@github-actions
Copy link

github-actions bot commented Nov 1, 2025

👍
No issues in PR Title / Commit Title

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the network configuration handling in the container run arguments generation by consolidating the network parameter construction into a single variable. Instead of having early returns with duplicated command construction, the code now builds a net variable that contains the appropriate network configuration and uses it in a single return statement.

  • Introduced a net variable to hold network configuration parameters
  • Replaced early return for host networking with conditional assignment to net
  • Unified the final return statement to use the net variable consistently

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions
Copy link

github-actions bot commented Nov 2, 2025

CI integration 👍 passed!

Details

CI logs

  • CLI Smoketests (no interfaces) 👍 passed
  • CLI Smoketests VPP 👍 passed
  • CLI Smoketests (interfaces only) 👍 passed
  • Config tests 👍 passed
  • Config tests VPP 👍 passed
  • RAID1 tests 👍 passed
  • TPM tests 👍 passed

Copy link
Member

@dmbaturin dmbaturin left a comment

Choose a reason for hiding this comment

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

I don't see any issues with the logic but I think the commit and PR title should clearly mark it container: and explain what arguments the change is about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

2 participants