Skip to content

Conversation

@SylvainSenechal
Copy link
Contributor

@SylvainSenechal SylvainSenechal commented Dec 16, 2025

Issue: CLDSRVCLT-4

"description": "Smithy-generated TypeScript client for Cloudserver's internal APIs",
"main": "dist/index.js",
"types": "dist/index.d.ts",
"exports": {
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

env:
SMITHY_VERSION: '1.61.0'
run: |
# Extract Smithy version from smithy-build.json
Copy link
Contributor Author

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

@SylvainSenechal SylvainSenechal requested review from a team, benzekrimaha and delthas December 16, 2025 17:00
@SylvainSenechal SylvainSenechal marked this pull request as ready for review December 16, 2025 17:00
@SylvainSenechal SylvainSenechal force-pushed the improvement/CLDSRVCLT-4 branch 4 times, most recently from 6e52335 to 3768de7 Compare December 17, 2025 14:34
import { CloudserverClientConfig } from '../../build/smithy/cloudserver/typescript-codegen';

export class BucketQuotaClient {
private client: CloudserverBucketQuota;
Copy link
Contributor

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 ?)

Copy link
Contributor Author

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 {
Copy link
Contributor

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

Copy link
Contributor Author

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';

Copy link
Contributor Author

@SylvainSenechal SylvainSenechal Jan 6, 2026

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"

Copy link
Contributor

@francoisferrand francoisferrand Jan 12, 2026

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 {
Copy link
Contributor

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) ?

Suggested change
operation UpdateBucketQuota {
operation PutBucketQuota {

Copy link
Contributor Author

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

Copy link
Contributor

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?

@SylvainSenechal SylvainSenechal force-pushed the improvement/CLDSRVCLT-7 branch 2 times, most recently from e09c5a5 to b433100 Compare December 19, 2025 10:24
@SylvainSenechal SylvainSenechal force-pushed the improvement/CLDSRVCLT-4 branch 5 times, most recently from d2aa0ac to e099139 Compare December 30, 2025 16:00
@SylvainSenechal SylvainSenechal force-pushed the improvement/CLDSRVCLT-7 branch from b433100 to ffa94b0 Compare January 5, 2026 10:27
Base automatically changed from improvement/CLDSRVCLT-7 to development/1.0 January 5, 2026 10:29
@bert-e
Copy link

bert-e commented Jan 5, 2026

Hello sylvainsenechal,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Available options
name description privileged authored
/after_pull_request Wait for the given pull request id to be merged before continuing with the current one.
/bypass_author_approval Bypass the pull request author's approval
/bypass_build_status Bypass the build and test status
/bypass_commit_size Bypass the check on the size of the changeset TBA
/bypass_incompatible_branch Bypass the check on the source branch prefix
/bypass_jira_check Bypass the Jira issue check
/bypass_peer_approval Bypass the pull request peers' approval
/bypass_leader_approval Bypass the pull request leaders' approval
/approve Instruct Bert-E that the author has approved the pull request. ✍️
/create_pull_requests Allow the creation of integration pull requests.
/create_integration_branches Allow the creation of integration branches.
/no_octopus Prevent Wall-E from doing any octopus merge and use multiple consecutive merge instead
/unanimity Change review acceptance criteria from one reviewer at least to all reviewers
/wait Instruct Bert-E not to run until further notice.
Available commands
name description privileged
/help Print Bert-E's manual in the pull request.
/status Print Bert-E's current status in the pull request TBA
/clear Remove all comments from Bert-E from the history TBA
/retry Re-start a fresh build TBA
/build Re-start a fresh build TBA
/force_reset Delete integration branches & pull requests, and restart merge process from the beginning.
/reset Try to remove integration branches unless there are commits on them which do not appear on the source branch.

Status report is not available.

@bert-e
Copy link

bert-e commented Jan 5, 2026

Incorrect fix version

The Fix Version/s in issue CLDSRVCLT-4 contains:

  • None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

  • 1.0.1

Please check the Fix Version/s of CLDSRVCLT-4, or the target
branch of this pull request.

registry-url: 'https://registry.npmjs.org'

- name: Publish to npm with provenance
run: npm publish --provenance --tag latest
Copy link
Contributor

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 }} ?

Suggested change
run: npm publish --provenance --tag ${{ needs.build.outputs.version }}

or even not be set at all ?

Suggested change
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.

Copy link
Contributor

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 {
Copy link
Contributor

@francoisferrand francoisferrand Jan 12, 2026

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 [...])

Copy link
Contributor

@francoisferrand francoisferrand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please rebase &

@SylvainSenechal SylvainSenechal force-pushed the improvement/CLDSRVCLT-4 branch 5 times, most recently from 7d23727 to d809ead Compare January 12, 2026 15:41
@SylvainSenechal SylvainSenechal force-pushed the improvement/CLDSRVCLT-4 branch from d809ead to 89416f7 Compare January 12, 2026 17:21
@SylvainSenechal SylvainSenechal force-pushed the improvement/CLDSRVCLT-4 branch from 89416f7 to a7c2a3c Compare January 12, 2026 17:33
{
"name": "@scality/cloudserverclient",
"version": "1.0.0",
"version": "1.1.0",
Copy link
Contributor

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants