-
Notifications
You must be signed in to change notification settings - Fork 0
Add bucket quota apis #5
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
Conversation
| "description": "Smithy-generated TypeScript client for Cloudserver's internal APIs", | ||
| "main": "dist/index.js", | ||
| "types": "dist/index.d.ts", | ||
| "exports": { |
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 will allow importing with subpath, for example
import { BucketQuotaClient } from '@scality/cloudserverclient/quota'
|
|
||
| namespace cloudserver.bucketquota | ||
|
|
||
| use aws.protocols#restXml |
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 have to create a new separate service, as the other client is json, but the quota apis are xml.
And anyways it might just be better for code organisation.
5170901 to
48729a4
Compare
| env: | ||
| SMITHY_VERSION: '1.61.0' | ||
| run: | | ||
| # Extract Smithy version from smithy-build.json |
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.
Not directly related with the pr, but this is nicer as we will only have one source of truth for smithy version.
I could definitely see someone updating smithy version here and forgetting about updating it in smithy-build.json
6e52335 to
3768de7
Compare
src/clients/bucketQuota.ts
Outdated
| import { CloudserverClientConfig } from '../../build/smithy/cloudserver/typescript-codegen'; | ||
|
|
||
| export class BucketQuotaClient { | ||
| private client: CloudserverBucketQuota; |
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.
ok to introduce a wrapper (c.f. previous point) ; however, this specific wrapper changes the overall "form" of the API, which does not look like AWS sdk v3 code : looks more like v2.
I wonder if it would not be better to wrap the command objects (GetBucketQuotaCommand, UpdateBucketQuotaCommand...) then send them as usual?
(this could also make it more seamless to have combined "s3" + quotas client: just need to write our own client whose send() method supports both s3 & quota api ?)
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 removed the wrapper, it should be clearer now
| @@ -0,0 +1,18 @@ | |||
| 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.
so now moving to separate clients for each service?
→ in that case, this specific client must be renamed ("BackeatRoutesClient" ?), as it does not cover all cloudserver routes...
even if each client is available "separately", may be good to keep an "aggregated" cloudserver client:
- for retro-compatibility, avoid breaking change (e.g. allow easily bumping in backbeat)
- for ease of use & discoverability
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.
Yeah I decided to split the client, anyways we are kinda forced to do it because of the json vs xml clients.
But also, as I added the other clients, I realized we start to have a few of them :
- quotas
- s3extended
- backbeat routes
- backbeat apis
And I believe most of the time, the utilisation of these apis will not be mixed. So now we have a clearer separation, and also the way these clients are exported, users will be able to import part of the client only (I added an example for bucket quota) :
import {
BucketQuotaClient,
CloudserverBucketQuotaClientConfig,
GetBucketQuotaCommand
} from '@scality/cloudserverclient/clients/bucketQuota';
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.
The only thing that I'm not very satisfied with, is the client named "cloudserver". The 3 others are quite clear in what they do
one is for quotas, one for the apis calling backbeat, and one for extending s3 regular apis.
But the "cloudserver" one, it contains 80% of backbeat routes //backbeat/.., but it turns out it also contains a few other things :
for example
//metadata/default/attributes
So now, I don't know exactly whats the best naming strategy for the cloudserver client, for now I kept it as "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.
I think CloudserverBackbeatRoutes is the way : a bit weird, but it is really what this is, for lack of anything better (the routes in cloudserver "designed" for backbeat)...
This would however break compatibility with (existing) backbeat? So we need to decide very soon, to avoid breaking...
|
|
||
| @http(method: "PUT", uri: "/{Bucket}?quota=true") | ||
| @idempotent | ||
| operation UpdateBucketQuota { |
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.
keep same conventions as AWS (and http/rest in general) ?
| operation UpdateBucketQuota { | |
| operation PutBucketQuota { |
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 kept the same names as the ones from the doc, do you confirm you wanna change it ?
Right now :
- cloudserver : bucketUpdateQuota
- documentation for clients : UpdateBucketQuota
- Here : UpdateBucketQuota
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 ask for a vote from the team in the channel?
e09c5a5 to
b433100
Compare
d2aa0ac to
e099139
Compare
b433100 to
ffa94b0
Compare
Hello sylvainsenechal,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
| registry-url: 'https://registry.npmjs.org' | ||
|
|
||
| - name: Publish to npm with provenance | ||
| run: npm publish --provenance --tag latest |
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 the tag not be ${{ needs.build.outputs.version }} ?
| run: npm publish --provenance --tag ${{ needs.build.outputs.version }} |
or even not be set at all ?
| run: npm publish --provenance |
I have not much experience with npm publish, but looking at the docs the default is latest already; and the docs explicitly states:
Publishing a package sets the latest tag to the published version unless the --tag option is used. For example, npm publish --tag=beta.
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.
(oops, wrong PR I guess : did you forget to rebase?)
| DeleteBucketQuotaCommandOutput, | ||
| } from '../../build/smithy/cloudserverBucketQuota/typescript-codegen'; | ||
|
|
||
| export class BucketQuotaClient extends CloudserverBucketQuotaClient { |
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.
can/should we have a dedicated "quota client", or somehow "merge" all clients to have a single one?
(i.e. have a generic "CloudserverClient which extends both S3Client, CloudserverBucketQuotaClient, CloudserverBackbeatRoutesClient [...])
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.
please rebase &
7d23727 to
d809ead
Compare
d809ead to
89416f7
Compare
89416f7 to
a7c2a3c
Compare
package.json
Outdated
| { | ||
| "name": "@scality/cloudserverclient", | ||
| "version": "1.0.0", | ||
| "version": "1.1.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.
1.1.0 should be on 1.1 branch...
783b446 to
7271bad
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
|
/approve |
|
I have successfully merged the changeset of this pull request
Please check the status of the associated issue CLDSRVCLT-4. Goodbye sylvainsenechal. The following options are set: approve |
Issue: CLDSRVCLT-4