Skip to content

utils: show error message when priority is missing - #452

Merged
jafingerhut merged 2 commits into
p4lang:masterfrom
rst0git:clean-up
Feb 24, 2022
Merged

jafingerhut merged 2 commits into
p4lang:masterfrom
rst0git:clean-up

Conversation

@rst0git

@rst0git rst0git commented Feb 23, 2022 •

Copy link
Copy Markdown
Member

This pull request adds a validation for P4Runtime table entries that shows an error message when the priority field is required but not specified. For example:

AssertionError: non-zero 'priority' field is required for all entries for table MyIngress.check_ports in pod-topo/s1-runtime.json

@rst0git
rst0git force-pushed the clean-up branch 3 times, most recently from 07e497f to 351c634 Compare February 23, 2022 22:00
Suggested-by: Antonin Bas <abas@vmware.com>
Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
A 'priority' field is required to order entries when the
table's match key includes an optional, ternary or range match.

Suggested-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>
Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
@jfingerh

Copy link
Copy Markdown
Contributor

If you do not mind, I would recommend having separate PRs for things like whitespace cleanup vs. Python import cleanups vs. new features. They are easier to review separately, rather than merged together.

@rst0git

rst0git commented Feb 24, 2022 •

Copy link
Copy Markdown
Member Author

@jfingerh Thank you Andy! I moved all cleanup changes in a draft pull requests (#453, #454) that are rebased on top of this one (to avoid merge conflicts). I can mark the draft pull requests as "ready for review" when this one has been merged.

match_types_with_priority = [
p4info_pb2.MatchField.TERNARY,
p4info_pb2.MatchField.RANGE
]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

also optional match_kind, yes?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I will go ahead and approve this, since this is easy to fix in a later separate PR, one that doesn't have two other open PRs based upon it already :-)

@jafingerhut jafingerhut left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, and I will open an issue to remind us to add match kind optional handling in a later PR.

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