-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
WIP: Secure Variables: Go API #13502
Conversation
angrycub
commented
Jun 27, 2022
•
edited
Loading
edited
- SV API: Go API
- Fix Testutil for delve debugging API tests
- Updates to API
- SV: Go API: Working state
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!
I see there are some TODO tests awaiting functionality to land on the RPC side? I think we can merge this into the feature branch and then follow-up with those tests later.
// delve debug executable for debugging test | ||
case strings.HasSuffix(path, "__debug_bin"): | ||
return false | ||
case strings.HasSuffix(path, "__debug_bin.exe"): | ||
return false |
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.
Nice!
api/api.go
Outdated
// closeResponseBody reads resp.Body until EOF, and then closes it. The read | ||
// is necessary to ensure that the http.Client's underlying RoundTripper is able | ||
// to re-use the TCP connection. See godoc on net/http.Client.Do. | ||
func closeResponseBody(resp *http.Response) error { | ||
_, _ = io.Copy(ioutil.Discard, resp.Body) | ||
return resp.Body.Close() | ||
} |
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.
This seems like a bug fix? Should we open a new PR with just this to fix the existing usage for backport as well?
api/secure_variables.go
Outdated
// CheckedUpdate is used to updated a secure variable if the modify index | ||
// matches the one on the server | ||
func (sv *SecureVariables) CheckedUpdate(v *SecureVariable, qo *WriteOptions) (*WriteMeta, 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 like the API split between checked and unchecked you've done here. It's much nicer than passing some random checked
bool flag or foisting the problem off on the consumer.
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |