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

Data Provider Skeleton Mockup #75

Draft
wants to merge 5 commits into
base: harry/add_data_provider
Choose a base branch
from

Conversation

harryrackmil
Copy link
Contributor

We'd like to make adding a new Data Provider source easier by auto-generating some skeleton code which the user can then fill in so they don't have to think too much about the framework.

In this PR I've manually written some suggested skeleton code for a new data source called skeleton, which can hopefully serve as a starting model for our templating

I think the ideal flow for a user would be a CLI-like tool where we ask them to give us the name of their dataSource in TitleCase. We would then automatically

  1. validate the string they gave us was valid (no underscores/spaces etc, not using the name of an existing dataSource)
  2. do some templating to auto-generate all the files in this PR (including the TODO comments) with their dataSource name in place ofskeleton. We can figure out camelCase,snake_case and flatcase versions of the dataSource name automatically and insert the appropriate one into the template.
  3. run python3 ./apps/scripts/update_shared_data_provider_code.py to update a couple of shared framework files
  4. Print a message to the user telling them to fill in all the TODOs (and maybe listing all the files containing a TODO)

Then the user just fills in the TODOs, gets their tests passing and submits the PR.

assert.Equal(t, skeleton.SkeletonDataSourceId, dataSourceId)

sourceSpecificConfig, err := skeleton.GetSourceSpecificConfig(sourceConfig)
assert.NoError(t, err)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

generate tests to make sure the config is valid and can be loaded, that we can extract the data source id from it, and that we can extract the SkeletonConfig.

The user just has to fill in a valid config string and write some asserts

import "github.com/Stork-Oracle/stork-external/apps/lib/data_provider/types"

type SkeletonConfig struct {
DataSource types.DataSourceId `json:"dataSource"` // required for all Data Provider Sources
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All configs need to have a DataSource field

The user just needs to add configuration details specific to their dataSource

}

func newSkeletonDataSource(sourceConfig types.DataProviderSourceConfig) *skeletonDataSource {
skeletonConfig, err := GetSourceSpecificConfig(sourceConfig)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a little opinionated, but it seems like any data source would need to be initialized using the data source's config.

Note that as written this skeleton code won't even compile, since this variable is unused. It might be a little nicer to have skeleton code which compiles cleanly but fails at runtime - this way seemed a little more readable to the developer though.

}

func (r skeletonDataSource) RunDataSource(updatesCh chan types.DataSourceUpdateMap) {
// TODO: Write all logic to fetch datapoints and add to updatesCh
Copy link
Contributor Author

Choose a reason for hiding this comment

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

super open ended

)

func TestSkeletonDataSource(t *testing.T) {
// TODO: write some unit tests for your data source
Copy link
Contributor Author

Choose a reason for hiding this comment

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

very open ended - didn't want to auto-generate unit tests that actually run the integration in case the integration hits external services

@@ -0,0 +1,30 @@
package skeleton
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this file is fully auto-generated - user doesn't need to touch it

Choose a reason for hiding this comment

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

maybe we can add a comment at the top of the file saying that it is autogenerated and to please not touch - similar to mockery

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm I'm not sure if we want to actually stop people from changing this file if they want to - it wouldn't be crazy for some advanced user to make changes to the factory code.

Copy link

@ACK101101 ACK101101 left a comment

Choose a reason for hiding this comment

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

i'm not 100% sure what i should be looking for but it seems like a good skeleton. should we provide an example implementation to make it even clearer?
also, what do you think about providing an explicit function in the flow for transformations? too opinionated?

@@ -0,0 +1,30 @@
package skeleton

Choose a reason for hiding this comment

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

maybe we can add a comment at the top of the file saying that it is autogenerated and to please not touch - similar to mockery

@@ -0,0 +1,12 @@
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed the //TODO comments from here because it's invalid json and was causing some framework tests to fail. We should probably explicitly say in the docs that users will need to edit this file once they write their config object.

If they try to configure anything other than data source id with this default schema, validation will fail. So hopefully that will force users to update this file even without the TODOs

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.

2 participants