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

Update for added context in grafana-aws-sdk #326

Merged
merged 5 commits into from
May 24, 2024

Conversation

njvrzm
Copy link
Contributor

@njvrzm njvrzm commented May 21, 2024

This PR adds context handling in a number of places to support the changes in the associated grafana-aws-sdk PR, which this one depends on. I've added a replace directive in go.mod pointing to that PR's branch to enable CI; that should be removed before merging.

This also replaces instances of context.TODO() in tests with context.Background(), so that the former remains a useful signal.

@njvrzm njvrzm requested a review from a team as a code owner May 21, 2024 14:12
@njvrzm njvrzm requested review from idastambuk, kevinwcyu and iwysiu May 21, 2024 14:12
@njvrzm njvrzm self-assigned this May 21, 2024
@njvrzm njvrzm changed the title Update for added sdk context Update for added context in grafana-aws-sdk May 21, 2024
go.mod Outdated
@@ -4,14 +4,16 @@ go 1.21

toolchain go1.21.0

replace github.com/grafana/grafana-aws-sdk => github.com/grafana/grafana-aws-sdk v0.25.2-0.20240521134039-65047541bce5
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: remove this before merging!

}

func (l Loader) LoadDriver(ctx context.Context, awsapi sqlAPI.AWSAPI) (awsDriver.Driver, error) {
return driver.New(ctx, awsapi)
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 keeps the existing behavior, but should it get a comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think at least the struct being labeled makes sense to explain what its for.

if err != nil {
return nil, err
}
return res.(*api.API), err
}

func (s *AthenaDatasource) CancelQuery(ctx context.Context, options sqlds.Options, queryID string) error {
api, err := s.getAPI(ctx, options)
athenaAPI, err := s.getAPI(ctx, options)
Copy link
Contributor Author

@njvrzm njvrzm May 21, 2024

Choose a reason for hiding this comment

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

Just unshadowing the api import, this and the many following instances

}

func (l Loader) LoadDriver(ctx context.Context, awsapi sqlAPI.AWSAPI) (awsDriver.Driver, error) {
return driver.New(ctx, awsapi)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think at least the struct being labeled makes sense to explain what its for.

@njvrzm njvrzm force-pushed the njvrzm/update_for_added_sdk_context branch from 4b6ebc6 to 7ecffbc Compare May 24, 2024 17:15
@njvrzm njvrzm merged commit d91146b into main May 24, 2024
4 checks passed
@njvrzm njvrzm deleted the njvrzm/update_for_added_sdk_context branch May 24, 2024 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants