Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Support SAi attr generation from annotattions #358

Merged
merged 6 commits into from
Apr 12, 2023

Conversation

marian-pritsak
Copy link
Collaborator

Add support for @Sai[] structured annotations for
table keys and action parameters. If not available, the old
way is still valid till all params and keys will be annotated.

@chrispsommers
Copy link
Collaborator

@marian-pritsak please sync to main, a cascade of PRs yesterday caused a regression in a test case, the fix was just merged minutes ago. The failure in your CI pipeline should go away.

Copy link
Collaborator

@chrispsommers chrispsommers left a comment

Choose a reason for hiding this comment

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

This is a worthwhile enhancement to the code generator, thanks. Please see comments regarding annotation naming conventions.

Also, It would be nice to start a README.md under dash-pipeline/bmv2 and start documenting some of our conventions, such as the proper usage of these annotations. We can build it up over time. This would be in line with #156.

Add support for @Sai[] structured annotations for
table keys and action parameters. If not available, the
old way is still valid till all params and keys will be
annotated.

Signed-off-by: Marian Pritsak <marianp@mellanox.com>
@chrispsommers
Copy link
Collaborator

@KrisNey-MSFT we can merge this

@marian-pritsak
Copy link
Collaborator Author

@chrispsommers is there a way for spellcheck to omit terms like "isresourcetype" and not flag them?

@chrispsommers
Copy link
Collaborator

@chrispsommers is there a way for spellcheck to omit terms like "isresourcetype" and not flag them?

The only way I know of is to enclose in backticks. Kind of a pain.

@reshmaintel
Copy link
Collaborator

Please add a description paragraph that can cover what is this change and why it is needed. This helps during review of the PRs. Thanks.

@marian-pritsak
Copy link
Collaborator Author

@KrisNey-MSFT This PR is ready for merging

@KrisNey-MSFT KrisNey-MSFT merged commit d50a995 into sonic-net:main Apr 12, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants