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

Move plugins to own package & improve apigen CLI #29

Closed
wants to merge 2 commits into from

Conversation

sknat
Copy link
Contributor

@sknat sknat commented Aug 11, 2022

This patch moves the binapigenerator plugins to their own package.

  • It capitalizes the methods they leverage.
  • It changes the way plugins are called, passing the whole generator
    at once and letting the plugin themselves loop on the files.
    This allow for more variety in filtering, aggregation, etc...

This patch also proposes an evolution to the binapigen CLI
in order to make it more user-friendly for newcomers as well
as build pipelines based on go:generate

For now two usecases are in the picture:

  • Building from a directory containing .json files
    e.g. 'binapigen --api /usr/share/vpp/api --output ./myapp/bindings --filter interface,ip'
  • Building from a cloned local VPP
    e.g. 'binapigen --vpp ~/vpp --output ./myapp/binding'

Signed-off-by: Nathan Skrzypczak nathan.skrzypczak@gmail.com
Change-Id: I5b0b2ade40ab80c9e91c2a422f8c193b232d9830

@sknat sknat changed the title Move plugins to own module & improve apigen CLI Move plugins to own package & improve apigen CLI Aug 11, 2022
@sknat sknat force-pushed the feature/improve-plugins-cli branch 6 times, most recently from fe96cac to 1461e33 Compare September 28, 2022 16:18
This patch adds a GetRetVal function on all reply
messages, that retreives the error value VPP
returns in its Reply. If the field is not present
or the value is 0 (success) it returns nil.

This also changes the types enum to xxxType names
e.g. api.RequestMessage becomes api.RequestMessageType

Signed-off-by: Nathan Skrzypczak <nathan.skrzypczak@gmail.com>
Change-Id: I55ca4d02b19909a3fd44c152962b934621646e9f
This patch moves the binapigenerator plugins to their own package.
- It capitalizes the methods they leverage.
- It changes the way plugins are called, passing the whole generator
at once and letting the plugin themselves loop on the files.
This allow for more variety in filtering, aggregation, etc...

This patch also proposes an evolution to the binapigen CLI
in order to make it more user-friendly for newcomers as well
as build pipelines based on go:generate

For now two usecases are in the picture:
- Building from a directory containing .json files
e.g. 'binapigen --api /usr/share/vpp/api --output ./myapp/bindings --filter interface,ip'
- Building from a cloned local VPP
e.g. 'binapigen --vpp ~/vpp --output ./myapp/binding'

Signed-off-by: Nathan Skrzypczak <nathan.skrzypczak@gmail.com>
Change-Id: I5b0b2ade40ab80c9e91c2a422f8c193b232d9830
@ondrej-fabry
Copy link
Member

Why including quite unrelated changes into here? It would be good to keep this PR only about separating the plugins - making the generator extendable. The changes to the API are not needed for this.

@sknat
Copy link
Contributor Author

sknat commented Oct 3, 2022

You mean having this patch on top of #28 or the fact that the 62f5161 in itself is too big in your view ?
Regarding the former, I'm keeping those two stacked as we are depending on the combination, and I wanted avoid the perpetual rebase overhead.

@ondrej-fabry
Copy link
Member

You mean having this patch on top of #28 or the fact that the 62f5161 in itself is too big in your view ? Regarding the former, I'm keeping those two stacked as we are depending on the combination, and I wanted avoid the perpetual rebase overhead.

I think both. Basically I think a change like this should be done in multiple steps. This helps with doing reviews, bisecting possible issues and makes the changes testable separately.

Overall, I do not see how these changes relate together in GoVPP and I believe these considerations must have priority over dependency/requirements of other projects.

@sknat
Copy link
Contributor Author

sknat commented Oct 17, 2022

Ok, so I did split this patch & the previous one, we have (in order)

All these patches are on top of each other (it's way easier due to dependencies between each of them), so looking at a PR will include changes in the previous ones until merged & rebased

@sknat sknat closed this Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants