Skip to content

T7282: op-mode: show firewall group filtering and tab completion update #4414

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.

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

Open
wants to merge 2 commits into
base: current
Choose a base branch
from

Conversation

markh0338
Copy link
Contributor

@markh0338 markh0338 commented Mar 25, 2025

Change summary

When showing firewall group , any dynamic groups are also shown along with the requested group name. Dynamic-groups, mac-groups and domain-groups are also not shown in tab completion.

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)

T7282

Related PR(s)

How to test / Smoketest result

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

@markh0338 markh0338 requested a review from a team as a code owner March 25, 2025 01:21
Copy link

github-actions bot commented Mar 25, 2025

👍
No issues in PR Title / Commit Title

@markh0338
Copy link
Contributor Author

Linting failed in the firewall.py file, but not in the section I modified. I can reformat the file if you’d like, but I wanted to keep the focus on the purpose of the PR instead of linting.

Copy link
Member

@c-po c-po 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 for the submission. Looks like you are mangling two changes into one commit which is - from a software development and life cycle point - not a good idea.

Can you please split this change into two individual commits?

  • Update TAB completion helpers
  • Fix the for loop by an early exit in the script showing the firewall rules?

@markh0338
Copy link
Contributor Author

Thank you for the submission. Looks like you are mangling two changes into one commit which is - from a software development and life cycle point - not a good idea.

Can you please split this change into two individual commits?

  • Update TAB completion helpers

  • Fix the for loop by an early exit in the script showing the firewall rules?

Just to clarify, would you want these completely split with different tasks and PRs? Or just different commits inside this PR?

@sever-sever sever-sever requested a review from c-po March 29, 2025 04:38
@c-po
Copy link
Member

c-po commented Mar 30, 2025

Please use individual commits inside a single PR referencing the same task id.

Copy link
Member

@c-po c-po left a comment

Choose a reason for hiding this comment

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

Implementation looks good to me

@markh0338
Copy link
Contributor Author

Rebased on current to sync up other firewall.py changes for remote-groups

Copy link
Member

@sever-sever sever-sever left a comment

Choose a reason for hiding this comment

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

Completion help is useful for firewall groups

@markh0338
Copy link
Contributor Author

before this gets merged - I think there is an issue with how the path tab completion works. Admittedly, I don't know how all of that works either currently but there is an issue if a group type isn't found it doesn't continue down the list. so in this commit, if a dynamic group isn't found, the rest won't process.

@markh0338
Copy link
Contributor Author

Completely overhauled the path completion logic. Previously, when using multiple entries in the completion help, a series of ls commands were chained together to list the directories. However, since these were executed in order, if the first directory had no groups defined, the remaining ls commands would not run—resulting in an incomplete group listing.

With the updated approach, each group directory is checked individually and only listed if it exists, ensuring more accurate and complete output.

@markh0338 markh0338 requested review from sever-sever and c-po April 9, 2025 04:13
Copy link

github-actions bot commented Apr 9, 2025

CI integration ❌ failed!

Details

CI logs

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

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.

3 participants