Skip to content

Conversation

@RubenNL
Copy link
Contributor

@RubenNL RubenNL commented Oct 28, 2025

Change Summary

So, I made a mistake in this PR: https://github.com/vyos/vyos.vyos/pull/403/files#diff-55d967f7efb21cec719c5ad71ac942a7769acf87c6e2747720fde15f62fa1fadR376.

When the commands is empty, this will throw a "index out of bounds" error.

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)

https://github.com/vyos/vyos.vyos/pull/403/files#diff-55d967f7efb21cec719c5ad71ac942a7769acf87c6e2747720fde15f62fa1fadR376

Component(s) name

firewall(_global)

Proposed changes

Just a simple len(commands) > 0 check.

How to test

Just like the test, create a group without anything in it (set firewall group EMPTY), and run a replaced firewall_global task.

Test results

Failing test: https://github.com/RubenNL/vyos.vyos/actions/runs/18937208873
Succeeding test: https://github.com/RubenNL/vyos.vyos/actions/runs/18937491823

  • Sanity tests passed
  • Unit tests passed

Tested against VyOS versions:

Checklist:

  • I have read the CONTRIBUTING document
  • I have linked this PR to one or more Phabricator Task(s)
  • I have run the ansible sanity and unit tests
  • My commit headlines contain a valid Task id
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have added unit tests to cover my changes
  • I have added a file to changelogs/fragments to describe the changes

@RubenNL RubenNL requested a review from a team as a code owner October 28, 2025 15:15
Copy link
Contributor

@gaige gaige left a comment

Choose a reason for hiding this comment

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

Do you have a reproduce case for the problem? That would be good to put in a test so that we can validate both the original failure and the fix here.

@RubenNL RubenNL force-pushed the T7964-fix-empty-commands-T7260 branch from 659b8ac to b03855c Compare October 30, 2025 10:25
@RubenNL
Copy link
Contributor Author

RubenNL commented Oct 30, 2025

Took a bit of time, but I managed to find the cause. It's just deleting a group using replace, when the group is empty (like set firewall group EMPTY).

Test has been added by force-pushing a commit with only the test.

Copy link
Contributor

@gaige gaige left a comment

Choose a reason for hiding this comment

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

Thank you! Very much appreciate you digging in and reporting, finding, and fixing!

Copy link
Contributor

@omnom62 omnom62 left a comment

Choose a reason for hiding this comment

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

Looks OK

@dmbaturin dmbaturin merged commit 1ccb6f9 into vyos:main Nov 3, 2025
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants