added s3 extended apis

[SylvainSenechal]

Issue: CLDSRVCLT-6

[francoisferrand]

this is called ListObjects in s3:

• should we not call this SearchObjects ?
• or even just keep the name ListObjects even if we have extra params for searching?

(similar for ListObjectsV2 and ListObjectVersions below)

[SylvainSenechal]

I reused the same names we had before for the migration, but I agree we can use official ones, and the frontend will adapt, they will have to change code anyway.
I will just add an "extended" keyword at the end to differentiate, otherwise its gonna be troublesome to differentiate official api from extended one :

ListObjects -> ListObjectsExtended

[francoisferrand]

maybe ListObjectsEx for brevity ? or ListObjectsScal / ScalListObjects ?

[francoisferrand]

can we have a single step, maybe something like:

"build:generated": "echo build/smithy/*/typescript-codegen | xargs 'cd {} && yarn install && yarn build'"

[SylvainSenechal]

I don't know current solution is longer but quite simple compared to this

[francoisferrand]

it is more error-prone, as we risk not-generating some modules, and need to call multiple "targets"

[francoisferrand]

do we really need these?

the same can be done easily from CLI: yarn test tests/testIndexesApis.test.ts for exemple...

[francoisferrand]

I understand not all tests work with every backend, but this approach:

• requires lots of "maintenance" when adding test suites
• has a high risk of breaking (forgetting to add a test suite & run it in CI)
• does not spare developer either, as they will run the same thing as CI... and we could miss issues altogether

Maybe a more scalable approach would be to "skip" some tests depending on the backend ? e.g.

• yarn test runs all tests
• the backend (mongo/metadata) is detected via environment variables or cloudserver api calls
• in the test code (in typescript), skip tests if they are not relevant for the backend
• also implement some extra test to verify the API fails with "not implemented" (I guess?) when used with the incorrect backend : e.g. metadata search or index with metadata backend, or metadata api with mongo...

[SylvainSenechal]

I added some kind of utility to override describe(), that will skip the test based on a new env variable

[francoisferrand]

Filtering tests a feature of jest : just need to add params ; and any params passed to yarn test is appended to the command of the script.

So we you have a script "test" : "jest",

• yarn test will run all tests with jest
• yarn test tests/testIndexesApis.test.ts will only run the tests in testIndexesApis.test.ts

i.e. carefully choosing the scripts and associated command gives lots of flexibility without any workaround...

[bert-e]

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.

[benzekrimaha]

As this is a listing I think we should find a better naming for this

[SylvainSenechal]

yep, I updated it

[bert-e]

Waiting for approval

The following approvals are needed before I can proceed with the merge:

• the author

• 2 peers

[francoisferrand]

can this be typed (avoid any)?

[SylvainSenechal]

Technically yes but the type here is very convoluted, requires importing stuff from aws sdk types that we don't really understand, I think it's ok this way

[francoisferrand]

Shoud we pass the whole input object? Is there no risk that AWS SDK "rejects" the query field?

[SylvainSenechal]

yeah its ok and tested, and I don't see what non complicated alternatives we would have

[francoisferrand]

don't we also support an extra X-Scal-Request-Uids ?
or should we have another API to set it (since we probably don't want to override every command) ?

[SylvainSenechal]

I'll rethink about this later when i do the follow up on backbeat where I will have to think about it.
But yeah I don't think it's the right place as we would need to override all commands, we will probably just use a middleware to add the request id if we want

[francoisferrand]

maybe an exercise for later, but I think we could factorize this, and use advanced typescript to "generate" the new class by adding a Query string param to the input of the constructor...

so we would end up with something like:

// Write the code just once here - may be a bit more complex though
func AddParameters(...) { }

export ListObjectsExtendedCommand = AddParameters(ListObjectsCommand, 'Query')
export ListObjectVersionsExtendedCommand = AddParameters(ListObjectVersionsCommand, 'Query')

(not required/blocking, and may not even work ; but I think this is possible and could help with maintenance..... though not sure at what cost)

[SylvainSenechal]

I just did a quick test with ai : it is possible to do something like this. If we had a lot of s3 command to exports it would probably be worth it, but here it just adding a lot of complexity (especially if we want to later handle multiple new parameters, + body vs query parameter) and doesn't seem worth it.

[francoisferrand]

I think maybe you can call skip from within the block, which may be friendly/readable: something like

descrube('Cloudsever Lifecycle API Tests', () => {
   if (process.env.BACKEND_TYPE === BackendType.METADATA) {
        skip(`${name} (tests skipped: mongo backend only)`);
   }

   it(..., () => {})
   it(..., () => {})
}

(it works with mocha, not sure about jest)

[SylvainSenechal]

I just tried this also, it's not super nice compared with existing solution because we have to do it in a before all, so we end up with 2 before all which i wanna avoid. Also with this, all the tests end up running, but are stopped early, so in the logs we see all tests as "run and skipped", instead of just skipping the whole test file with the describe

[SylvainSenechal]

/approve

[bert-e]

I have successfully merged the changeset of this pull request
into targetted development branches:

• ✔️ development/1.0

Please check the status of the associated issue CLDSRVCLT-6.

Goodbye sylvainsenechal.

The following options are set: approve