-
Notifications
You must be signed in to change notification settings - Fork 2
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
ft: BKTCLT-18 bootstrap go client #256
Conversation
jonathan-gramain
commented
Sep 11, 2024
- Initialize a new Go module with "bucketclient" package
- BucketClient and BucketClientError types
- Add a generic Request() function to query bucketd and retrieve the raw response
Hello jonathan-gramain,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command:
Alternatively, the |
go-client/Makefile
Outdated
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.
should this be in the same git repo (→ better locallity), or if we should use a new repo/project (→ simpler, mono-language repo ; better integration with golang package management or other tooling, ...) ?
@rachedbenmustapha what do you think?
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'm not against a new repository if that helps with what you mentioned, no strong opinion on my side.
The same suggestion may apply for the migration tools (https://github.com/scality/MetaData/pull/2250) which could have their own repository as well, as there is no code dependency with the Metadata JS code (they depend on bucketd to be available on the same machine but that doesn't preclude using a different repository). Beside the better integration with golang package management, it would also avoid running all Metadata tests when pushing to it (and vice-versa). We may still integrate the built binaries into the Metadata image, or create a new migration image to run it inside a new container (running along with the scality-bucketd-migration
containers).
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'm also down for another repo or a mono repo for go and js packages.
Otherwise it could be nice to filter out the go files for js package with a files
property in package.json
or .npmignore
(but I'm not sure it works when installing from url instead of a registry) and same for go
run: make lint | ||
|
||
- name: Run unit tests | ||
run: make test-coverage |
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.
we probably want to take the change to activate codecov
already, so we don't accumulate debt while building up the project (c.f. https://github.com/scality/sorbet/blob/HEAD/codecov.yml and https://github.com/scality/sorbet/blob/HEAD/.github/workflows/tests.yaml#L119-L126 for exemple)
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 do next week, separately from this PR (https://scality.atlassian.net/browse/BKTCLT-20). I made a PR for metadata-migration: https://github.com/scality/metadata-migration/pull/8
af78c7e
to
5a8e27b
Compare
d5b0131
to
c356dea
Compare
c356dea
to
962ceb4
Compare
go/bucketclientrequest_test.go
Outdated
defer cancel() | ||
|
||
_, err := client.Request(timeoutCtx, "GetObject", "GET", "/foo/bar") | ||
Expect(err).To(MatchError(ContainSubstring("context deadline exceeded"))) |
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.
Expect(err).To(MatchError(ContainSubstring("context deadline exceeded"))) | |
Expect(err).To(MatchError(context.DeadlineExceeded)) |
go/bucketclientrequest.go
Outdated
idempotent IdempotentOpt | ||
} | ||
|
||
func parseRequestOptions(opts ...any) (requestOptionsSet, 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.
A common pattern to avoid untyped apis for variable options is to use functions:
type RequestOption func(*requestOptionSet)
func RequestBodyOption(body RequestBodyOpt) RequestOption {
return func(ros *requestOptionSet) {
ros.requestBody = body
}
}
func Idempotent(ros *requestOptionSet) {
ros. idempotent = true
}
func parseRequestOptions(opts ...RequestOption) (requestOptionsSet, error) {
for _, o := range opts {
o(&parsedOpts)
}
}
...
parseRequestOptions(RequestBodyOption(body), Idempotent)
...
This way it's impossible to pass an unknown/invalid option.
Also instead of parsing and returning a struct to be consumed by the Request
function, the options (functions) could be directly applied by Request
if they took an http request as an argument to mutate.
Add a generic Request() function to query bucketd and retrieve the raw response
Updates to Go client only
0b3e7f2
to
871d3ac
Compare
/approve |
ConflictA conflict has been raised during the creation of I have not created the integration branch. Here are the steps to resolve this conflict: $ git fetch
$ git checkout -B w/8.1/feature/BKTCLT-18-go-bindings origin/development/8.1
$ git merge origin/feature/BKTCLT-18-go-bindings
$ # <intense conflict resolution>
$ git commit
$ git push -u origin w/8.1/feature/BKTCLT-18-go-bindings The following options are set: approve |
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue BKTCLT-18. Goodbye jonathan-gramain. The following options are set: approve |