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

157-Make-module-apis-more-ergonomic #206

Closed
wants to merge 1 commit into from

Conversation

azenna
Copy link
Contributor

@azenna azenna commented Sep 21, 2023

Make module api more erogonomic

Use a module trait to build interface for pulsar modules

Implementation

Should be working, definitely needs some cleanup though

I have

  • run cargo fmt;
  • run cargo clippy;
  • run cargo testand all tests pass;
  • linked to the originating issue (if applicable).

resolves #157

@azenna azenna force-pushed the 157-more-ergonomic-module-apis branch from a1f60e8 to 23813bc Compare September 21, 2023 08:58
@banditopazzo banditopazzo self-assigned this Sep 21, 2023
@banditopazzo
Copy link
Member

hi, sorry it took me a long time to think about the original issue. It's not a simple problem because it's a complete refactoring of all the internals and there are a lot of things to think about before going to the development.

I have seen your pr and there are few wrong things, for example there are both the Module trait and the PulsarModule struct.

Another example is the _ = rx_config.changed() => {...} branch in the tokio:select! of module.rs, it's used to check if the configuration changed but modules need to parse the configuration themselves in their on_change function.

Or yet another example is that the bus receiver is always allocated in module.rs, even for for modules that don't use it.

Apart from that, we have discussed internally and there are other upcoming changes we have in the pipeline (for example changes to the configuration format and to the threat response, etc). So we need a deeper refactoring to address all these needs.

I will update the issue in the next days with other details.

Feel free to join the discussion. Thanks

@azenna azenna marked this pull request as draft September 26, 2023 12:59
@azenna
Copy link
Contributor Author

azenna commented Sep 26, 2023

heyy, yeah completely understand! I'll work on addressing your feedback in this pr over the next couple of days. Mostly just wanted to work on this to see more of the project and learn new things. Let me where the PR needs to move and I'll work on it!

@krsh krsh closed this Apr 15, 2024
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.

proposal: more ergonomic apis for modules
3 participants