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.

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

lib: remove leaf-list xpath hack from northbound #15196

Merged
merged 1 commit into from
Jan 24, 2024

Conversation

idryzhov
Copy link
Contributor

@idryzhov idryzhov commented Jan 23, 2024

Currently, when editing a leaf-list, nb_candidate_edit expects to receive its xpath without a predicate and the value in a separate argument, and then creates the full xpath. This hack is complicated, because it depends on the operation and on the caller being a backend or not. Instead, let's require to always include the predicate in a leaf-list xpath. Update all the usages in the code accordingly.

Copy link
Contributor

@choppsv1 choppsv1 left a comment

Choose a reason for hiding this comment

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

How did you verify that you caught all the leaf-list use cases?

Currently, when editing a leaf-list, `nb_candidate_edit` expects to
receive it's xpath without a predicate and the value in a separate
argument, and then creates the full xpath. This hack is complicated,
because it depends on the operation and on the caller being a backend or
not. Instead, let's require to always include the predicate in a
leaf-list xpath. Update all the usages in the code accordingly.

Signed-off-by: Igor Ryzhov <[email protected]>
@idryzhov
Copy link
Contributor Author

I grepped all leaf-lists we have in our models and modified related CLI commands. Actually I forgot one in zebra, because I thought it was converted to NB only in my pending PR, but it was already converted in master. Just pushed the fix.

@donaldsharp donaldsharp merged commit 16406a3 into FRRouting:master Jan 24, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants