-
Notifications
You must be signed in to change notification settings - Fork 193
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
Wimortl/sql #116
Wimortl/sql #116
Conversation
* improve tests with parallel execution and rm sleep * fix the tests to run on kindcluster
* feat: implement keyvault controller * Ace's KV changes with updates * Added an event for the final successful provisioning * Updated changes based on the PR comments * removing unwanted file * making resource group name the one in the keyvault yaml Co-authored-by: Ace Eldeib <alexeldeib@gmail.com>
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.
Looks good. Had a few comments, main thing is to make the SDK calls async
New test output: Williams-MBP:sqlclient wmortl$ go test |
…service-operator into wimortl/sql
@frodopwns I fixed the small stuff @jananivMS all async changes are in |
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.
let's merge this and address any other issues with a new pr
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.
LGTM
return false, fmt.Errorf("cannot create sql server: %v", err) | ||
} | ||
|
||
return true, nil |
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.
Will, I am going to approve this for now, but we should discuss about if we should change this to return the result (gotten from future.Result) which would confirm that the async operation is in progress.
The support for signal handling was already hooked up but we didn’t check for cancellation anywhere. With this change we check for cancellation: * During JSON Schema loading: for each HTTP request or file open. * While cleaning the output directory: once per file or directory to be cleaned, also while walking the tree. * While converting the JSON AST: once per handler. * While emitting the output files: once per package. The JSON Schema library doesn’t support cancellation so this is implemented there by supplying a filesystem implementation that supports cancellation; there is no way to supply an implementation for the HTTP requests so instead we add cancellation to the global HTTP transport (in `gen.go`). We should try and push a change upstream to either add cancellation overall or allow overriding the HTTP client/transport. Fixes #116.
What this PR does / why we need it:
sql server client
Special notes for your reviewer:
If applicable: