Skip to content

Conversation

@jestabro
Copy link
Contributor

@jestabro jestabro commented Oct 28, 2025

Change summary

NOTE: this requires
vyos/vyos1x-config#54
vyos/vyconf#33
and an update to this PR to point to those repos after merge. Integration tests will not run until the first of those is merged and pointed to.
<-- prerequisites merged and hash references updated

The addition of alert exn to vyos1x-config allows an audit of all raised exceptions, providing fixes for consistent handling and error reporting.

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): compiler driven exception audit

Related Task(s)

Related PR(s)

vyos/vyos1x-config#54
vyos/vyconf#33

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

@jestabro jestabro self-assigned this Oct 28, 2025
@github-actions
Copy link

github-actions bot commented Oct 28, 2025

👍
No issues in PR Title / Commit Title

@jestabro
Copy link
Contributor Author

Move to draft to fix overly-restrictive treatment of Useless_set as error: this has been seen in automated tests --- need to allow redundant set operations.

@jestabro jestabro marked this pull request as ready for review October 28, 2025 23:11
@jestabro jestabro force-pushed the exn-alert branch 2 times, most recently from 817482e to 12975ed Compare October 29, 2025 15:41
@jestabro
Copy link
Contributor Author

Added: expose value_exists for future use (cf. T7967) and rebased to separate commit under task T7969 --- it is included in this collection of PRs as it is required internally (and was already present) for
vyos/vyconf#33

Note that this has now a hard requirement on vyos/vyos1x-config#54 so CI integration is not possible until that is merged.

@jestabro
Copy link
Contributor Author

jestabro commented Nov 4, 2025

Rebased over current and resolved conflicts.

@github-actions
Copy link

github-actions bot commented Nov 4, 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

@sever-sever sever-sever requested a review from Copilot November 5, 2025 11:33
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 pull request improves error handling in the VyOS configuration tree library by enhancing error messages and adding better exception handling. The changes make error reporting more consistent and informative throughout the library.

Key Changes:

  • Added value_exists() function to check if a specific value exists at a path
  • Enhanced error handling to retrieve and display detailed error messages from the underlying OCaml library
  • Improved return value checking for is_leaf() to properly return boolean values
  • Updated dependency versions for vyos1x-config and vyconf libraries

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
python/vyos/configtree.py Added value_exists() method and improved error handling with detailed messages from underlying library
libvyosconfig/lib/bindings.ml Enhanced exception handling with proper error message setting and alert annotations for all exceptions
libvyosconfig/Makefile Updated dependency commit hashes for vyos1x-config and vyconf libraries

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

The addition of alert exn to vyos1x-config allows an audit of all raised
exceptions, providing fixes for consistent handling and error reporting.
@github-actions
Copy link

github-actions bot commented Nov 5, 2025

CI integration ❌ failed!

Details

CI logs

  • CLI Smoketests (no interfaces) ❌ failed
  • 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 cannot find any objections against this logic.

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.

Update hash and fix to warnings.

I trust @jestabro on this.

Also adds nice value_Exists utility function

@dmbaturin dmbaturin merged commit c21b066 into vyos:current Nov 6, 2025
17 of 18 checks passed
@vyosbot vyosbot added mirror-initiated This PR initiated for mirror sync workflow mirror-completed and removed mirror-initiated This PR initiated for mirror sync workflow labels Nov 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants