-
Notifications
You must be signed in to change notification settings - Fork 16
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
VAL-369: Use postgres in e2e #194
VAL-369: Use postgres in e2e #194
Conversation
09b9f17
to
da5001b
Compare
55b54ff
to
d62f372
Compare
4453d41
to
9b49d97
Compare
9b49d97
to
f14ee77
Compare
env: | ||
PROVIDERS_URLS: ${{ secrets.PROVIDERS_URLS }} | ||
CL_API_URLS: "https://e2e-test.lido.fi," |
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.
why ?
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.
maybe instead we should set VALIDATOR_REGISTRY_ENABLE=false
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 code will fail due to spread operators for CL_API_URLS
and PROVIDERS_URLS
, if some of them are missing:
lido-keys-api/src/common/config/config.service.ts
Lines 9 to 13 in 9d800c9
public get secrets(): string[] { | |
return [this.get('SENTRY_DSN') ?? '', ...this.get('CL_API_URLS'), ...this.get('PROVIDERS_URLS')] | |
.filter((v) => v) | |
.map((v) => String(v)); | |
} |
Perhaps, this PR is not a good place to change this. We can create a task for that.
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 strange, we have VALIDATOR_REGISTRY_ENABLE=false specially not to provide CL_API_URLS
i dont think it will fail
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.
yes, i checked , it will not fail
you could add this code in config.service.ts get function to check
and add in .env
VALIDATOR_REGISTRY_ENABLE=false without providing CL_API_URLS
if (key == 'CL_API_URLS') {
console.log('CL_URL', value);
console.log(
'secrets',
[super.get('SENTRY_DSN') ?? '', ...super.get('CL_API_URLS'), ...super.get('PROVIDERS_URLS')]
.filter((v) => v)
.map((v) => String(v)),
);
}
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.
did you add VALIDATOR_REGISTRY_ENABLE=false ?
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 important to add VALIDATOR_REGISTRY_ENABLE=false for such setup
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.
@ValidateIf((e) => e.VALIDATOR_REGISTRY_ENABLE === true)
@IsArray()
@ArrayMinSize(1)
@Transform(({ value }) => value.split(',').map((url) => url.replace(/\/$/, '')))
CL_API_URLS: 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.
i see, for validators tests it is impossible to set VALIDATOR_REGISTRY_ENABLE = false
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.
sorry, before i mocked config , so it worked, now you are right
@@ -34,7 +34,9 @@ | |||
"test:watch": "jest --watch", | |||
"test:cov": "jest --coverage", | |||
"test:debug": "node --inspect-brk -r tsconfig-paths/register -r ts-node/register node_modules/.bin/jest --runInBand", | |||
"test:e2e": "jest --config jest-e2e.json && chronix test", | |||
"test:e2e": "docker-compose --env-file=./.env -f docker-compose.e2e.yml up --build --abort-on-container-exit", | |||
"test:e2e:docker": "mikro-orm schema:drop -r && mikro-orm migration:up && jest -i --config jest-e2e.json && chronix test", |
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.
why do we name it test:e2e:docker ?
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.
"test:e2e:docker": "mikro-orm schema:drop -r && mikro-orm migration:up && jest -i --config jest-e2e.json && chronix test - is a test running command
docker-compose --env-file=./.env -f docker-compose.e2e.yml up --build --abort-on-container-exit - container running
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 can name it test:e2e:ci
, WDYT?
…eat/VAL-369-use-postgres-in-e2e-1
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.
lgtm
8d63546
into
feat/val-255-sr-modules-operators-keys-stream-2
No description provided.