-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add more context handling #139
Conversation
2832cfa
to
b14075d
Compare
@@ -13,17 +13,17 @@ steps: | |||
- name: build | |||
image: grafana/grafana-plugin-ci:1.9.5 | |||
commands: | |||
- mage -v build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the build steps were randomly failing, I believe because they share a docker volume and one step's mage
process is deleting the mage_output_file.go
before another step is done with it. This change appears to fix it.
GetDB(ctx context.Context, id int64, options sqlds.Options) (*sql.DB, error) | ||
GetAsyncDB(ctx context.Context, id int64, options sqlds.Options) (awsds.AsyncDB, error) | ||
GetAPI(ctx context.Context, id int64, options sqlds.Options) (api.AWSAPI, error) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Athena and redshift were defining this interface for themselves, which seemed backwards to me; rather than update both copies with context arguments I've moved it here.
LoadAPI(context.Context, *awsds.SessionCache, models.Settings) (api.AWSAPI, error) | ||
LoadDriver(context.Context, api.AWSAPI) (driver.Driver, error) | ||
LoadAsyncDriver(context.Context, api.AWSAPI) (asyncDriver.Driver, error) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love this approach but I like it better than passing around a bunch of typed functions. I'm open to alternatives :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think its better (probably equivalent tbh) but you could make a struct of the options and pass that around?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would work! I think you're right though that it's not really better. Gonna keep it this way since it's already done.
LoadAPI(context.Context, *awsds.SessionCache, models.Settings) (api.AWSAPI, error) | ||
LoadDriver(context.Context, api.AWSAPI) (driver.Driver, error) | ||
LoadAsyncDriver(context.Context, api.AWSAPI) (asyncDriver.Driver, error) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think its better (probably equivalent tbh) but you could make a struct of the options and pass that around?
For compatibility with grafana-plugin-sdk (specifically changes in v0.177.0 and v0.182.0) we're adding
context.Context
to more places. This also brings in an interface that two client datasources (athena and redshift) have been defining identically for themselves, and streamlines the various loaders that were being passed around into a single interface thatdatasource.New
will expect. Finally,context.TODO()
is replaced withcontext.Background()
in tests.This is a breaking change, so the next release should be >= v0.26.