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

feat: added preliminary SDK support to "rand_data" plugin #427

Merged
merged 2 commits into from
Sep 20, 2024

Conversation

patrickjcasey
Copy link
Contributor

@patrickjcasey patrickjcasey commented Sep 19, 2024

This pull request is the first pass at using the hipcheck-sdk crate to make a plugin for hipcheck

The goal of the SDK is to make it as easy as possible for plugin authors to write new plugins for hipcheck!

The hidden hc plugin command no accepts another flag --sdk that will use the dummy_rand_data_sdk plugin, rather than the dummy_rand_data plugin

There are some outstanding issues that still need to be addressed, whether in this issue, or in another issue in the future:

  • How are we handling third-party dependencies that are exposed in public facing types (serde_json::Value, schemars::schema::SchemaObject, ...)
  • How much documentation should be present in the hipcheck-sdk crate
  • How do we test the hipcheck-sdk crate to ensure it is producing the desired behaviors and integrates cleanly with what hipcheck is expecting
  • Does it make sense to add a prelude module so a user would be able to use hipcheck_sdk::prelude::* and have the most common imports?

@alilleybrinker
Copy link
Collaborator

Thanks @patrickjcasey! I'll take a look tomorrow! 😄

sdk/rust/src/error.rs Outdated Show resolved Hide resolved
sdk/rust/src/lib.rs Outdated Show resolved Hide resolved
@j-lanson
Copy link
Collaborator

Re: a prelude crate

As far as I see, excluding the re-exports the only types users need access to is QuerySchema, DynQuery, NamedQuery, Query, Plugin, PluginServer, PluginEngine, plus the error module. And anyone using the SDK to write a plugin will need all of these.

I'd be in favor of adding a prelude crate so that users that want to do so can simply import * as you have shown above.

Leave this and the reexport crate as a separate commit on top for now in case we need to walk it back

@j-lanson
Copy link
Collaborator

As for documentation and testing, those are important, but they can be done after the SDK is merged

@patrickjcasey patrickjcasey force-pushed the jlanson/rust-sdk-experimental branch 2 times, most recently from d1f88b3 to f007d93 Compare September 20, 2024 15:45
@alilleybrinker
Copy link
Collaborator

I concur on the idea of a prelude module.

Copy link
Collaborator

@j-lanson j-lanson left a comment

Choose a reason for hiding this comment

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

This looks good to me! Great work

@patrickjcasey patrickjcasey merged commit 0036c7c into main Sep 20, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants