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

Update remainder of regex pattern statements for XSD conformance #534

Merged
merged 6 commits into from
May 25, 2022

Conversation

earies
Copy link
Contributor

@earies earies commented Dec 6, 2021

  • (M) release/models/bgp/openconfig-bgp-policy.yang
  • (M) release/models/openflow/openconfig-openflow-types.yang
  • (M) release/models/policy/openconfig-routing-policy.yang
  • (M) release/models/vlan/openconfig-vlan-types.yang
  • (M) release/models/wifi/openconfig-ap-manager.yang
    • Remove anchors in pattern statements to make them W3C compliant
    • Remove regex-posix extension from these updated modules

This PR covers a few artifacts remaining through the models where YANG pattern
statements were not updated to reflect the XSD regex variants. In addition to
the regex updates are removal of the oc-ext:regexp-posix flavor extension
statements.

NOTE: This PR only covers the modules where pattern statements were still
broken. An additional PR will cover removal of oc-ext:regexp-posix across all
modules.

* (M) release/models/bgp/openconfig-bgp-policy.yang
* (M) release/models/openflow/openconfig-openflow-types.yang
* (M) release/models/policy/openconfig-routing-policy.yang
* (M) release/models/vlan/openconfig-vlan-types.yang
* (M) release/models/wifi/openconfig-ap-manager.yang
  - Remove anchors in pattern statements to make them W3C compliant
  - Remove regex-posix extension from these updated modules
@google-cla google-cla bot added the cla: yes Commit status indicating that CLA is on file label Dec 6, 2021
@OpenConfigBot
Copy link

OpenConfigBot commented Dec 6, 2021

Compatibility Report for commit 756e20f:
yanglint@SO 1.10.17

@dplore
Copy link
Member

dplore commented Dec 14, 2021

LGTM
oc-ext:regexp-posix is no longer used, these modules were missed from earlier cleanup
All other changes are removing ^ % from the regex strings

Copy link
Contributor

@wenovus wenovus left a comment

Choose a reason for hiding this comment

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

So the reason why these were left unconverted is because we wanted to add unit testing for them prior to changing them. e.g. https://github.com/openconfig/models/pull/920/files

If these don't have unit tests, could we open a tracking issue to add them?

@earies
Copy link
Contributor Author

earies commented Mar 22, 2022

So the reason why these were left unconverted is because we wanted to add unit testing for them prior to changing them. e.g. https://github.com/openconfig/models/pull/920/files

If these don't have unit tests, could we open a tracking issue to add them?

Thx @wenovus but you are referencing a private repo that many folks don't have access to so cannot see what this entails

@wenovus
Copy link
Contributor

wenovus commented Mar 22, 2022

So the reason why these were left unconverted is because we wanted to add unit testing for them prior to changing them. e.g. https://github.com/openconfig/models/pull/920/files
If these don't have unit tests, could we open a tracking issue to add them?

Thx @wenovus but you are referencing a private repo that many folks don't have access to so cannot see what this entails

Ah apologies, here is an example from the public repo: https://github.com/openconfig/public/pull/460/files

@wenovus
Copy link
Contributor

wenovus commented Mar 22, 2022

The PR is closed, but was merged upstream.

The reason why we were waiting for unit tests is that there are some differences between XSD regex and POSIX regex aside from the anchors. Considering I never noticed any in practice during the conversion I think there is a good likelihood this will work, I just want to make sure if we're doing this to unblock some folks that we're tracking that the tests will be added at some point.

@dplore
Copy link
Member

dplore commented May 24, 2022

@earies you could add a yang regex validator here: https://github.com/openconfig/public/tree/master/regexp-tests

Wen confirmed with me that you can make that validator change part of this same PR and the CI checks will execute it as part of your PR together. BTW, placing a comment with the following text will force the CI to run again:

/gcbrun

@earies
Copy link
Contributor Author

earies commented May 25, 2022

@earies you could add a yang regex validator here: https://github.com/openconfig/public/tree/master/regexp-tests

Wen confirmed with me that you can make that validator change part of this same PR and the CI checks will execute it as part of your PR together. BTW, placing a comment with the following text will force the CI to run again:

/gcbrun

Will update accordingly but note that any inline pattern statements (e.g. non-reusable types) cannot be covered here without complete duplication of the string pattern in the test which is a de-synchronized exercise I don't think we want to do in this fashion (masklength-range is one such example here)

  * (A) regexp-tests/openconfig-bgp-policy-test.yang
  * (A) regexp-tests/openconfig-openflow-types-test.yang
  * (A) regexp-tests/openconfig-openflow-vlan-types-test.yang
@earies
Copy link
Contributor Author

earies commented May 25, 2022

@dplore @wenovus @aashaikh - note there are a few issues w/ regexp tests

  • 2 nodes in this patch set have inline pattern statements. These types of tests cannot accommodate so there will be no coverage for such (There is lacking coverage already in other modules for this) - From this patch set it is masklength-range and country-code
  • For typedefs in non -types modules means an import of that module which cascades dependencies in this case from openflow -> interfaces -> ietf-interfaces and ietf-interfaces is not an OpenConfig module thus fails the regexp tests

It seems the regexp framework would need some additional work and don't see that as blocking the fixup of these remaining invalid regexes. The types module tests included in here can remain but the policy test module would need to be removed

@wenovus
Copy link
Contributor

wenovus commented May 25, 2022

@dplore @wenovus @aashaikh - note there are a few issues w/ regexp tests

  • 2 nodes in this patch set have inline pattern statements. These types of tests cannot accommodate so there will be no coverage for such (There is lacking coverage already in other modules for this) - From this patch set it is masklength-range and country-code
  • For typedefs in non -types modules means an import of that module which cascades dependencies in this case from openflow -> interfaces -> ietf-interfaces and ietf-interfaces is not an OpenConfig module thus fails the regexp tests

It seems the regexp framework would need some additional work and don't see that as blocking the fixup of these remaining invalid regexes. The types module tests included in here can remain but the policy test module would need to be removed

Thanks for the work here, SGTM let's not block the tests on these two types of regexes, feel free to comment out the non-working tests and create an issue assigning to me and I will look at enabling them.

@earies
Copy link
Contributor Author

earies commented May 25, 2022

  • 2 nodes in this patch set have inline pattern statements. These types of tests cannot accommodate so there will be no coverage for such (There is lacking coverage already in other modules for this) - From this patch set it is masklength-range and country-code

  • For typedefs in non -types modules means an import of that module which cascades dependencies in this case from openflow -> interfaces -> ietf-interfaces and ietf-interfaces is not an OpenConfig module thus fails the regexp tests

PR updated and passing tests w/ coverage for the -types module changes

Issue #648 opened for tracking regexp test enhancements. @dplore @wenovus - direct assignment of issues/PRs is restricted so you may want to consider opening this up for easier tagging

Copy link
Contributor

@wenovus wenovus left a comment

Choose a reason for hiding this comment

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

LGTM, I'll leave to Darren to determine whether the test cases have sufficient coverage.

Copy link
Member

@dplore dplore left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Commit status indicating that CLA is on file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants