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.

Sign up for GitHub

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

Jump to bottom

T6817 & T6825 & T7004 updates - fw_rules override and replaced fixes #368

Merged
merged 42 commits into from
Jan 25, 2025
Merged

T6817 & T6825 & T7004 updates - fw_rules override and replaced fixes #368

merged 42 commits into from
Jan 25, 2025
+2,592 −678

Conversation

omnom62
Copy link
Contributor

@omnom62 omnom62 commented Dec 18, 2024 •
edited
Loading

Change Summary

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)

Component(s) name

Proposed changes

How to test

Test results

  • Sanity tests passed
  • Unit tests passed

Tested against VyOS versions:

  • 1.3.8
  • 1.4-rolling-202201010100

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

@omnom62 omnom62 changed the title T6817 updates - fw_rules override fixes T6817 & T6825 updates - fw_rules override and replaced fixes Dec 26, 2024
@gaige
Copy link
Contributor

gaige commented Dec 27, 2024

You'll need to add the changelog file (note: you can't just amend another file, it needs a new file to pass the tests).

@omnom62 omnom62 marked this pull request as ready for review January 8, 2025 04:56
@omnom62 omnom62 requested review from gaige and a team January 8, 2025 04:59
gaige
gaige requested changes Jan 14, 2025
View reviewed changes
plugins/module_utils/network/vyos/config/firewall_rules/firewall_rules.py Outdated Show resolved Hide resolved
plugins/module_utils/network/vyos/config/firewall_rules/firewall_rules.py Outdated Show resolved Hide resolved
tests/unit/modules/network/vyos/test_vyos_firewall_rules.old Outdated Show resolved Hide resolved
tests/unit/modules/network/vyos/test_vyos_firewall_rules14.py Outdated Show resolved Hide resolved
tests/unit/modules/network/vyos/test_vyos_firewall_rules.old Outdated Show resolved Hide resolved
tests/integration/targets/vyos_firewall_rules/tests/cli/parsed.yaml Outdated Show resolved Hide resolved
tests/integration/targets/vyos_firewall_rules/tests/cli/overridden.yaml Outdated Show resolved Hide resolved
tests/integration/targets/vyos_firewall_rules/tests/cli/deleted_afi.yaml Outdated Show resolved Hide resolved
tests/integration/targets/vyos_firewall_rules/tests/cli/_remove_config.yaml Outdated Show resolved Hide resolved
tests/integration/targets/vyos_firewall_rules/tests/cli/_populate.yaml Outdated Show resolved Hide resolved
@omnom62 omnom62 changed the title T6817 & T6825 updates - fw_rules override and replaced fixes T6817 & T6825 & T7004 updates - fw_rules override and replaced fixes Jan 14, 2025
@omnom62 omnom62 requested a review from gaige January 21, 2025 03:08
gaige
gaige requested changes Jan 23, 2025
View reviewed changes
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.

Extremely close. There are a couple of yaml syntax changes and a couple of disabled tests we should look at and one new line of code that isn't being tested with either unit or integration tests.

Code coverage in the main modules changed here are over 80% and every new line (except the one above) is covered in either integration or unit, so that's great!

changelogs/fragments/firewall_rules.yml Outdated Show resolved Hide resolved
plugins/module_utils/network/vyos/config/firewall_rules/firewall_rules.py Show resolved Hide resolved
tests/integration/targets/vyos_firewall_rules/vars/main.yaml Outdated Show resolved Hide resolved
tests/integration/targets/vyos_firewall_rules/vars/main.yaml Outdated Show resolved Hide resolved
tests/unit/modules/network/vyos/test_vyos_firewall_rules13.py Outdated Show resolved Hide resolved
tests/unit/modules/network/vyos/test_vyos_firewall_rules13.py Outdated Show resolved Hide resolved
@omnom62 omnom62 requested a review from gaige January 23, 2025 20:40
gaige
gaige approved these changes Jan 24, 2025
View reviewed changes
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.

LGTM

@gaige gaige requested a review from a team January 24, 2025 03:05
andamasov
andamasov approved these changes Jan 25, 2025
View reviewed changes
Copy link
Member

@andamasov andamasov left a comment

Choose a reason for hiding this comment

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

Good to go

@gaige gaige merged commit d0c73e6 into vyos:main Jan 25, 2025
60 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andamasov andamasov andamasov approved these changes

@gaige gaige gaige approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@omnom62 @gaige @andamasov