Bump Cli-testing to 1.3.0, update kafka libraries#2341
Bump Cli-testing to 1.3.0, update kafka libraries#2341SylvainSenechal wants to merge 20 commits intodevelopment/2.14from
Conversation
Hello sylvainsenechal,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
50ff85e to
b441c6e
Compare
b0358e1 to
e39e7a2
Compare
| "build": "tsc --build tsconfig.json", | ||
| "lint": "eslint ." | ||
| }, | ||
| "resolutions": { |
There was a problem hiding this comment.
related to issues newer azure libraries : https://scality.atlassian.net/browse/ZENKO-5213
There was a problem hiding this comment.
This was an infinite loop, although catched by global test timeout, still nicer to properly control while loops
There was a problem hiding this comment.
These are all small syntax changes because I did a huge bump of the kubernetes client from version 0.x to 1.4.0
| * new `putBucketNotificationConfiguration` to the connector before the | ||
| * test proceeds to trigger events. | ||
| */ | ||
| export async function waitForBucketInConnectorPipeline( |
There was a problem hiding this comment.
Used in bucket notification : Before, we were doing
- putBucketNotificationConfiguration
- sleep for 10sec and pray the oplog has picked up the change and updated kafka connector.
It was risky, because triggering the notification event before kafka connector was setup would imply the notification would never be sent
Now we detect the kafka connector is really setup.
I'm kinda sad though : I thought this would fix bucket notif flaky, but it doesn't, so there are other things to check
| assert.strictEqual(this.getResult().err?.includes('AccessDenied') || | ||
| this.getResult().err?.includes('403'), true); | ||
| this.getResult().err?.includes('403')|| | ||
| this.getResult().statusCode === 403, true); |
There was a problem hiding this comment.
For some reason, I got issues with this part not catching the 403 anymore.
Not fully sure why, but with the bump of the cli testing, the version of the sdk has increased quite a lot, from ~3.900 to ~3.100, maybe something changed I don't know
| Given that lifecycle is "resumed" for the "e2e-azure-archive" location | ||
| Then object "obj-1" should be "transitioning" and have the storage class "e2e-azure-archive" | ||
| And object "obj-2" should be "transitioning" and have the storage class "e2e-azure-archive" | ||
| # This test is flaky, and doesn't make much sense as it is : |
There was a problem hiding this comment.
@francoisferrand Let's discuss this. I only commented for now, maybe we wanna keep it ?
But looking at the test it doesnt make much sense (cf the comment I added in the code).
But at the same time I feel like I'm missing something, there must be a reason why this test was created
There was a problem hiding this comment.
Don't forget to resolve this before merging 😉
| cleanupAccount, | ||
| } from './utils'; | ||
|
|
||
| import 'cli-testing/hooks/KeycloakSetup'; |
There was a problem hiding this comment.
We have BeforeAll in Cli-testing, no import => they are not run
Imo should be in this repo but its a story for another day
| POD_NAME="${ZENKO_NAME}-ctst-tests" | ||
| CTST_VERSION=$(sed 's/.*"cli-testing": ".*#\(.*\)".*/\1/;t;d' ../../../tests/ctst/package.json) | ||
|
|
||
| # Configure keycloak |
There was a problem hiding this comment.
Seed keycloak is now done in a before all from Cli-testing
| import { safeJsonParse } from './utils'; | ||
| import assert from 'assert'; | ||
| import { Admin, Kafka } from 'kafkajs'; | ||
| import { Admin } from '@platformatic/kafka'; |
There was a problem hiding this comment.
Switched the kafka library
Before we were using kafkajs, and node-rdkafka.
These internally relied on librdkafka, a C library. Because of that, we needed to setup quite a few extra stuff to make them work : node-gyp (which was throwing weird python errors when doing yarn install in the Codespace), and also need a bunch of things to be installed in the dockerfile (you'll see later in the pr, librd-XXX).
Now this library is pure js, it removes some dependency, and also it will be compatible with Bun if we wanna use it later
There was a problem hiding this comment.
notes:
- such comments would be better in the PR comment, so reviewers know before even reading the code
- would be good to keep such change in a separate PR (even if it means stacking multiple PRs) - not saying to change it now (too late), but as a goal for next changes
There was a problem hiding this comment.
Thanks for the kafka change!
| const tarName = await isObjectRehydrated(this, objectName); | ||
| assert(tarName); | ||
| await AzureHelper.sendBlobCreatedEventToQueue( | ||
| const sent = await AzureHelper.sendBlobCreatedEventToQueue( |
There was a problem hiding this comment.
Lost crazy amount of time on this :
sendBlobCreatedEventToQueue was erroring, the error wasn't thrown but instead the function return a boolean that was never checked -_-
e39e7a2 to
dedf6c3
Compare
| const low = earliestResult[0]?.partitions.find(p => p.partitionIndex === i)?.offset ?? BigInt(0); | ||
| const high = latestResult[0]?.partitions.find(p => p.partitionIndex === i)?.offset ?? BigInt(0); |
There was a problem hiding this comment.
partitions are lists, right? So searching is not efficient...
- are partitions "ordered" predictably?
- do earliest and latest have the same order?
- ...could we not just do a "zip" merge, something like:
for (let i = 0; i < partitionCount; i++) {
partitions.push({ low: earliestResult[0]?.partitions[I], high: latestResult[0]?.partitions[I] });
There was a problem hiding this comment.
I think we don't have guarantee about the order, but the list should be small enough that in practice this find is not too impactfull
| offset: msg.offset?.toString(), | ||
| value: msg.value, | ||
| }); | ||
| try { |
There was a problem hiding this comment.
nested try blocks (3 here!) are kind of hard to read, should we not introduce helper functions?
| const eventTypeMatches = notificationType === notification?.eventName; | ||
| if (bucketNameMatches && objectNameMatches && eventTypeMatches) { | ||
| return true; | ||
| try { |
There was a problem hiding this comment.
instead of integrating the lib here, should we modify KafkaHelper directly in cli-testing to change the kafka lib?
or at least we need to remove that code (and dependency) from cli-testing?
There was a problem hiding this comment.
I removed the kafka helper from cli-testing, it was only used once in Zenko, no need for such a helper, and this way, no more node-rdkafka in cli testing
| format: [ | ||
| 'progress-bar', | ||
| '@cucumber/pretty-formatter', | ||
| 'pretty', |
There was a problem hiding this comment.
why are we removing @cucumber/pretty-formatter ?
seems a good option to auto-format .freature files?
There was a problem hiding this comment.
Pretty formatter is archived (https://github.com/cucumber/cucumber-js-pretty-formatter?tab=readme-ov-file), It's integrated directly in cucumber now (thats why i removed pretty formatter in cli-testing), and pretty formatter is now just "pretty" : https://github.com/cucumber/cucumber-js/blob/main/docs/formatters.md
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
4a27e41 to
0c6fedd
Compare
tests/ctst/Dockerfile
Outdated
| FROM ghcr.io/scality/zenko-drctl:$DRCTL_TAG AS drctl | ||
|
|
||
| FROM node:22.19.0-bookworm-slim | ||
| FROM node:24-bookworm-slim |
There was a problem hiding this comment.
Shouldn't we pin the version like before ?
There was a problem hiding this comment.
yeah gonna use 24.14
| Given that lifecycle is "resumed" for the "e2e-azure-archive" location | ||
| Then object "obj-1" should be "transitioning" and have the storage class "e2e-azure-archive" | ||
| And object "obj-2" should be "transitioning" and have the storage class "e2e-azure-archive" | ||
| # This test is flaky, and doesn't make much sense as it is : |
There was a problem hiding this comment.
Don't forget to resolve this before merging 😉
| // S3 is correct. | ||
| assert.strictEqual(this.getResult().err?.includes('AccessDenied') || | ||
| this.getResult().err?.includes('403'), true); | ||
| this.getResult().err?.includes('403')|| |
There was a problem hiding this comment.
Do we really need to keep this condition ?
There was a problem hiding this comment.
I am not 100% sure anymore tbh, might have been something that failed while i was working, but probably only need one of the two now.
But a/b testing by launching the ci again with and without i too long to just fix this
Issue: ZENKO-5203
…he DLQ was ~250 in the tests, stopping sorbetclt from restoring objects Issue: ZENKO-5203
…rs instead of sleep Issue: ZENKO-5203
…prove debugging Issue: ZENKO-5203
0c6fedd to
879ebee
Compare
Issue: ZENKO-5203
Ended up adding some extra stuff as I was testing my changes in Codespace and found a few stuff bothering me. I think the pr is still not too hard to review.
Can review commit by commit too