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

Feature/builtin plugins only #107

Closed
wants to merge 3 commits into from

Conversation

steweg
Copy link
Contributor

@steweg steweg commented Mar 3, 2024

No description provided.

@steweg steweg force-pushed the feature/builtin-plugins-only branch 2 times, most recently from d7b86a2 to 1de8c85 Compare March 3, 2024 21:50
@steweg steweg marked this pull request as ready for review March 3, 2024 21:51
Copy link
Collaborator

@rjarry rjarry left a comment

Choose a reason for hiding this comment

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

Hi,

libyang-python only supports the master branch of C libyang (currently 2.x).

The lydevel check is only meant for informative purposes.

libyang-python/tox.ini

Lines 18 to 20 in f09ed11

[testenv:lydevel]
setenv =
LIBYANG_BRANCH=devel

If it fails (currently it should fail because libyang 3.x is under development) it is not a problem.

Could you remove all references to libyang 3 which was not released yet and only focus on the enhancements that you require?

Thank you.

@steweg
Copy link
Contributor Author

steweg commented Mar 29, 2024

Hi,

libyang-python only supports the master branch of C libyang (currently 2.x).

The lydevel check is only meant for informative purposes.

libyang-python/tox.ini

Lines 18 to 20 in f09ed11

[testenv:lydevel]
setenv =
LIBYANG_BRANCH=devel

If it fails (currently it should fail because libyang 3.x is under development) it is not a problem.

Could you remove all references to libyang 3 which was not released yet and only focus on the enhancements that you require?

Thank you.

Unfortunately again, this feature requires to use libyang 3.0.X, so this has to wait.

@steweg steweg force-pushed the feature/builtin-plugins-only branch 5 times, most recently from 4b10649 to e579191 Compare April 6, 2024 10:29
@steweg steweg requested a review from rjarry May 9, 2024 10:47
@samuel-gauthier
Copy link
Collaborator

It tried the first commit and it does not seem to work (the unit test gives me a validation error). If you still want this integrated, could you rebase and recheck? Also, why do we need the last commit for, as it not seem related to the two others?

This patch introduces store_only flag, which allows user to only store
value without performing any sort of additional type checks as
length, range, pattern, etc.

Signed-off-by: Stefan Gula <[email protected]>
@steweg steweg force-pushed the feature/builtin-plugins-only branch from e579191 to 1c6919b Compare August 3, 2024 07:38
This patch adds builtin_plugins_only option, which allows user to use
only basic YANG type plugins, instead of using advanced plugins as
ipv4-address etc.

Signed-off-by: Stefan Gula <[email protected]>
This patch fixes broken APIs, which were using no longer valid structs and
performing validation step instead of just checking the realtype of data

Signed-off-by: Stefan Gula <[email protected]>
@steweg
Copy link
Contributor Author

steweg commented Aug 3, 2024

Rebased as requested

@samuel-gauthier
Copy link
Collaborator

Also, why do we need the last commit, as it does not seem related to the two others?
Is it because of the previous changes? Can you explain in more detail, I don't understand why we need it.

@steweg
Copy link
Contributor Author

steweg commented Aug 5, 2024

It is indirectly related. The thing is that once you want to use feature like using of built in plugins only and store only, then you are able to create dnode which passes only very basic checks (like that value is a string type) but skipping the advanced checks (like matching regexps etc.). This works nicely but then when you want to print the data, it will fail as the current printing procedure is also performing value validation against full set of checks, which is totally unexpected behavior from end-user perspective.

samuel-gauthier pushed a commit that referenced this pull request Sep 27, 2024
This patch fixes broken APIs, which were using no longer valid structs
and performing validation step instead of just checking the realtype of
data.

Closes: #107
Signed-off-by: Stefan Gula <[email protected]>
Signed-off-by: Samuel Gauthier <[email protected]>
@samuel-gauthier
Copy link
Collaborator

I found the time to fix the last commit, which could sometimes stay forever in the while loop.

@steweg
Copy link
Contributor Author

steweg commented Sep 27, 2024

I found the time to fix the last commit, which could sometimes stay forever in the while loop.

Are you sure this works even for "union of union of union..." and also for "leafref pointing to another leafref, pointing to another leafref.."?

Also I cannot really imagine a valid YANG schema, which will trigger infinite loop. Only "looped" YANG schema can do that, which shall not even pass through schema validation in general. If it does, then there is a bug in libyang schema validation code and shall be fixed rather there, not in this function.... Can you give me an example of such schema?

@samuel-gauthier
Copy link
Collaborator

With the code from your commit, our internal tests were looping.
The committed code is similar to what is done in lyd_value_validate, without the validation part, which does what you requested. And it works the same as before, as lyd_value_validate was called before.
Let's reopen this discussion in another issue if you find a problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants