Skip to content

Conversation

@wenovus
Copy link
Contributor

@wenovus wenovus commented Jan 7, 2021

The demo workflow is not required to pass -- its purpose is to show what the test results would be if it were run on the current public models.

Example failing (table) output: https://gist.github.com/OpenConfigBot/a4af6cb5cab50719b08017e93faa577b
Example passing output: https://gist.github.com/OpenConfigBot/75654c2ce460f11169acc06f37f1b1aa

@wenovus wenovus changed the title Run pattern_test.sh in Github Actions to demo change Tablefy Output and Add Demo CI Workflow Jan 8, 2021
@wenovus wenovus marked this pull request as ready for review January 8, 2021 00:59
@wenovus wenovus requested a review from robshakir January 8, 2021 00:59
Copy link
Member

@robshakir robshakir left a comment

Choose a reason for hiding this comment

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

One minor change, but this LGTM.

I assume that the 'fail' output is just a demo that will be reverted before submission?

@wenovus
Copy link
Contributor Author

wenovus commented Jan 8, 2021

One minor change, but this LGTM.

I assume that the 'fail' output is just a demo that will be reverted before submission?

The demo is allowed to fail. This is because patterns must be added prior to being used in models-ci, and that means a breaking change CAN cause an expected failure. If there is a failure, however, the current policy (see updated README), is to update the minor version.

@wenovus wenovus requested a review from robshakir January 8, 2021 16:07
@robshakir
Copy link
Member

I'm confused -- if the demo is allowed to fail, then it probably should not be something that we're running in the checks that are to be run for a PR.

If we're doing a negative test here, then make the "success" of the check be that the test is failing - that way you can still check that the failure is caught correctly, but not end up with the scenario where it looks like checks are failing for the PR.

@wenovus
Copy link
Contributor Author

wenovus commented Jan 11, 2021

I'm confused -- if the demo is allowed to fail, then it probably should not be something that we're running in the checks that are to be run for a PR.

If we're doing a negative test here, then make the "success" of the check be that the test is failing - that way you can still check that the failure is caught correctly, but not end up with the scenario where it looks like checks are failing for the PR.

OK that makes sense to me, I've made the run passing now. It will still be part of the release process to make sure that with a breaking new test, that the minor version must be incremented. If unsure, simply increment the minor version and update in the model repos.

@wenovus wenovus requested a review from aashaikh January 14, 2021 16:57
@wenovus
Copy link
Contributor Author

wenovus commented Jan 14, 2021

@aashaikh Adding you for review of the "Demo" CI checks. If you open the one called Demo on openconfig/public / pattern statement, for the "Demo output on openconfig/public" step, you can see how the current set of tests perform on the current openconfig/public models. This serves as a way of verifying the effects of the tests on the current regexes. Maintainers of this repo would then increment the minor version in order to prevent a failure from occurring on the models repos until they're approved separately via a version increment on the models repos.

This is the workflow I'm proposing for adding new tests/changing tests for existing regexes. I haven't thought about how to deal with adding new regexes yet. I think Rob is largely ok with the proposal, but want to get your double check here.

@wenovus
Copy link
Contributor Author

wenovus commented Feb 5, 2021

I'll assume that it's ok that the "Demo" CI steps pass despite regex failures. We can change this if this turns out to not be desired. I think the exact output from "Demo" that we're expecting has to be manually checked, so an improvement would be to post a comment or a link to a gist on what the failure messages are. I've opened #8 to track that.

@wenovus wenovus merged commit 784a1d1 into main Feb 5, 2021
@wenovus wenovus deleted the table-output branch February 5, 2021 16:52
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.

2 participants