-
Notifications
You must be signed in to change notification settings - Fork 0
Built cloudserver client with Smithy #1
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
base: development/1.0
Are you sure you want to change the base?
Conversation
920e18a to
356c165
Compare
legacy/backbeat-2017-07-01.api.json
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.
I think its nice to have this here as a reference, let me know if you think we should remove it
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.
it is still in (backbeat) git history, should be enough I think.
would be nice however to link to backbeat file on the PR message and/or commit message
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.
👍 agree to delete it
8101fc0 to
103d2ed
Compare
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 "build" folder be committed?
looking at package.json, it seems we rebuild them anyway...
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 need to have smithy installed to generate the build folder, I feel like it would be more complex if it's not comited ? At what moment would the build generation happen ?
I'm not too sure about this
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 be in the CI to be honest and the pushed with the release
| @restJson1 | ||
| @sigv4(name: "s3") | ||
| @service(sdkId: "cloudserver") | ||
| service cloudserver { |
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.
looking at the bigger picture (i.e. all the APIs supported by cloudserver), this is only one part : i.e. the "Backbeat routes".
eventually (not in this PR ;) we should have other APIs I guess, i.e.
- custom listing and other "extensions" to s3 api with extra attributes/... (typically done with a middleware, not sure if/how we want to override or just provide an ready-to-use middleware)
- metadata routes https://github.com/scality/cloudserver/blob/development/9.1/lib/routes/routeMetadata.js
- backbeat api routes (as used in zenkoclient, i.e. just frontend for backbeat/api extension)
- quota API
- ...
→ given this context, is cloudserver the right name for this specific service?
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.
ping
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.
IMO cloudserver is fine. It's a first iteration so we don't have all of them and I think it's ok. Maybe we should add a TODO.md with which API is missing, the cleanup of the tests (assert of process.env) and the CI? And link them to JIRA ticket
legacy/backbeat-2017-07-01.api.json
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.
it is still in (backbeat) git history, should be enough I think.
would be nice however to link to backbeat file on the PR message and/or commit message
tests/package.json
Outdated
| }, | ||
| "dependencies": { | ||
| "@aws-sdk/client-s3": "^3.896.0", | ||
| "@scality/cloudserverclient": "file:../build/smithy/source/typescript-codegen" |
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 works in the same repo (
file:..); how will it work from another repo, i.e. with git reference? - depending on the "internal" generation path of the package does not look very good..... could we/should we "wrap" this with a top-level file -maybe manually generated- re-exporting tuff from
build/smithy/source/typescript-codegen? So that other packages could directly use the package.json defined at root directory?
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.
alternatively, we may push the registry... c.f. scality/Guidelines#206
(may be done in a followup PR, adding github actions workflows to build/... everything)
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 tried importing it from backbeat
It worked with this :
"@scality/cloudserverclient": "file:../cloudserverclient"
What I did here in the package.json is if you look at "main", "types", "modules", I provided the path to the build folder :
"main": "build/smithy/source/typescript-codegen/dist-cjs"
So when we release a version of this repo, we should be able to use the library this way :
"@scality/cloudserverclient": "github:scality/cloudserverclient#v1.0.0"
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.
Ended up changing it :
We now have a client wrapper setup at the root project, it was needed to be able to provide this client with the error wrapper needed to handle html/xml error parsing
francoisferrand
left a comment
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.
publishing some overall comments, did not yet review the tests or models yet
c8442a6 to
c6c998f
Compare
766fbca to
125bc0d
Compare
tests/testRaftApis.test.ts
Outdated
| const raftIdData = await client.send(getRaftIdCommand); | ||
| console.log('GetRaftId succeeded:', raftIdData); | ||
| console.log('Raft ID:', raftIdData.RaftId); | ||
| } catch (err) { |
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 should not catch the error, as it prevents the test framework from noticing : so the test will always succeed...
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.
(if this is expected, best to just skip the test for now...)
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.
Ideally these tests should be run in a pipeline as functional tests, for now I ran them manually and I think they are fine this way. I have another ticket for setting up a pipeline, although I think this will not be straightforward to test : We need cloudserver to be started with different setup depending on the api we want to test
0e5f323 to
dfc7d0f
Compare
src/index.ts
Outdated
| }); | ||
| } | ||
|
|
||
| // S3C has an nginx proxy that can return HTML error responses |
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.
|
|
||
| list LocationMDList { | ||
| member: LocationMDObj | ||
| } | ||
|
|
||
| structure LocationMDObj { | ||
| /// Storage key for this location | ||
| key: String, | ||
| /// Size of the data stored at this location | ||
| size: Integer, | ||
| /// Start position/offset for this data segment | ||
| start: Integer, | ||
| /// Name of the data store where this is located | ||
| dataStoreName: String, | ||
| /// Type of the data store (e.g., file, mem, etc.) | ||
| dataStoreType: String, | ||
| /// ETag from the data store for this location | ||
| dataStoreETag: String, | ||
| /// Version ID in the data store for this location | ||
| dataStoreVersionId: String | ||
| } |
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.
nit: these are shared types, can they be moved to a separate smithy file? (locationMD.smithy)
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.
done
| blob StreamingBlob | ||
|
|
||
| /// Uploads a part for a multipart upload to multiple backend storage | ||
| @idempotent |
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.
does this not create a new part on every call, making it non-idempotent?
| output: GetObjectOutput, | ||
| } | ||
|
|
||
| structure GetObjectInput { |
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.
is this not the same type used in AWS ?
could we import those types and inherit from them (for custom/extra fields we support)?
something like:
structure GetObjectInput extends aws.GetObjectInput {
@httpHeader("X-Scal-Request-Uids")
RequestUids: String
}
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.
it look similar as the regular getObject input but it doesn't seem possible to do such an "extend". I prefer to keep this api fully custom. What we should've maybe looked at is if we can remove this api and use a classic getObject
| $version: "2.0" | ||
| namespace cloudserver.client | ||
|
|
||
| @http(method: "POST", uri: "/_/backbeat/index/{Bucket}?operation=delete") |
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.
don't we need this:
@idempotent
@suppress(["HttpMethodSemantics.UnexpectedPayload"])
(it has a body in the definition, and delete should be idempotent...)
(or maybe there should not be a body?)
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 added idempotent, suppress not needed, there is no warning as the api verb is not delete/put
| "target": "es2020", | ||
| "module": "commonjs", | ||
| "lib": ["es2020"], | ||
| "outDir": "./dist", |
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.
nit: should we output to ./buid instead, so all the generated code is in a single place?
|
LGTM, some cleanup (extra space / missing line in models...) to do. Not sure what is the effect of |
| @@ -0,0 +1,105 @@ | |||
| import { | |||
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.
As we are in a test folder, we can just use test.apis.ts or test.apisjs.js. spec.ts is used when test file are located next to the src ?
| @restJson1 | ||
| @sigv4(name: "s3") | ||
| @service(sdkId: "cloudserver") | ||
| service cloudserver { |
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.
IMO cloudserver is fine. It's a first iteration so we don't have all of them and I think it's ok. Maybe we should add a TODO.md with which API is missing, the cleanup of the tests (assert of process.env) and the CI? And link them to JIRA ticket
|
Checklist:
|
d4469f0 to
f58afc2
Compare
f58afc2 to
3d727eb
Compare
Related : scality/backbeat#2679
Replacing the "Backbeat client" built with the AWS SDK V2 in Backbeat, with this new client built with Smithy, related to our migration to the AWS SDK V3
ISSUE: CLDSRVCLT-1