Conversation
Hello sylvainsenechal,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
1047fe5 to
e3583d0
Compare
|
I will clean my commits after reviews |
There was a problem hiding this comment.
Trying to minimize the utilisation of global env variable that are used only once, or sometimes even useless, there is more work to do on this topic but this will be for other pr
| --env=AZURE_BLOB_URL=$AZURE_BACKEND_ENDPOINT \ | ||
| --env=AZURE_QUEUE_URL=$AZURE_BACKEND_QUEUE_ENDPOINT \ | ||
| --env=VERBOSE=1 \ | ||
| --env=SDK=true \ |
There was a problem hiding this comment.
Cli testing can be run in sdk mode, or CLI mode.
This shouldn't have impact, because actually this env used to be set in the Dockerfile of the E2E_IMAGE.
I put it here instead, because imo putting it in the Dockerfile was kinda hidding this parameter, and we are already defining all the other envs variable here
| ] | ||
| } | ||
| }' -- ./run "$COMMAND" $WORLD_PARAMETERS --parallel $PARALLEL_RUNS --retry $RETRIES --retry-tag-filter @Flaky --format junit:$JUNIT_REPORT_PATH "$@" | ||
| }' -- yarn cucumber-js \ |
There was a problem hiding this comment.
Imo much better, before it was hard to see, but a lot of things were happening behind the ./run commands, which wasn't even defined in Zenko, but in Cli-testing.
Here, we are now just naturally using cucumber syntax to run cucumber tests
.github/workflows/end2end.yaml
Outdated
There was a problem hiding this comment.
this is what I was talking about when I said I struggled to deal with cli-testing auth. It only worked when I added the ~/.netrc thing, which kinda looks smelly
There was a problem hiding this comment.
which command failed in which context (codespace, ci, inside docker image...) ?
- we need to use the GIT_ACCESS_TOKEN to have access to other repos (instead of GitHub usual reds, which are restricted the current repo + public repos)
- the git config above is already a trick to inject the GIT_ACCESS_TOKEN in git urls (so we don't need to modify the url in every place it is used: package.json, ...)
- netrc is an alternative to configure git auth, which should not be needed if the git config are made (but maybe we miss some, etc...)
| @@ -667,7 +663,7 @@ | |||
There was a problem hiding this comment.
Also nicer, bye bye """"""""""""", and now we can clearly see which tags are triggered (yeah cucumber allows to use multiple tags with and/or syntax)
There was a problem hiding this comment.
This is the equivalent of what we had in Cli-testing too : https://github.com/scality/cli-testing/blob/development/1.3/Dockerfile
I kept it mostly the same, but defined it here, so that cli-testing is used as a library, and not something where we store dockerfiles, commands, configs for Zenko.
There was a problem hiding this comment.
Many packages here can be updated to a higher version, but let's keep it for another pr
| # certain tests based on their @version tag. | ||
| VERSION=$(cat ../../VERSION | grep -Po 'VERSION="\K[^"]*') | ||
| POD_NAME="ctst-end2end" | ||
| WORLD_PARAMETERS="$(jq -c <<EOF |
There was a problem hiding this comment.
This is where we still have frictions : The current setup is basically enough to run tests locally, but these tests needs a lot of env variables that are passed as "world parameter". Here, I used a few that I defined manually to make one test work.
Later gonna work on using kube client to get secrets directly at the beginning of the tests, this will give us 2 things :
- Reduce setup needed to run test locally
- Reduce and refactor the number of things happening in .sh setup files. Final objective is also to have variables defined only once at the same place
There was a problem hiding this comment.
seems now we are running as a regular node program: so it should work as well without docker/pod?
→ maybe some tests (and world) need to be adapted, but we should try to investigate what changes it would require (which commands to tweak, ...). Esp. we already need special tricks to run kubectl commands, so it may actually simplify stuff...
There was a problem hiding this comment.
Yes, we may get there eventually, this should be doable, but probably lots of variables related to endpoint needs to change with port forward
There was a problem hiding this comment.
agreed this is not for this PR
port forward
in most case we use (or should use) the ingress : so no port forward...
we may however need to tweak etc/host or some other trick for DNS resolution, though
56485d6 to
a71ddc4
Compare
.github/workflows/end2end.yaml
Outdated
There was a problem hiding this comment.
which command failed in which context (codespace, ci, inside docker image...) ?
- we need to use the GIT_ACCESS_TOKEN to have access to other repos (instead of GitHub usual reds, which are restricted the current repo + public repos)
- the git config above is already a trick to inject the GIT_ACCESS_TOKEN in git urls (so we don't need to modify the url in every place it is used: package.json, ...)
- netrc is an alternative to configure git auth, which should not be needed if the git config are made (but maybe we miss some, etc...)
tests/ctst/README.md
Outdated
There was a problem hiding this comment.
latest does not work since we have multiple branches : so should always be rebuilt (maybe use an image tag which is unique to the branch, but not sure how to build images & tag them so it is practical...)
| # certain tests based on their @version tag. | ||
| VERSION=$(cat ../../VERSION | grep -Po 'VERSION="\K[^"]*') | ||
| POD_NAME="ctst-end2end" | ||
| WORLD_PARAMETERS="$(jq -c <<EOF |
There was a problem hiding this comment.
seems now we are running as a regular node program: so it should work as well without docker/pod?
→ maybe some tests (and world) need to be adapted, but we should try to investigate what changes it would require (which commands to tweak, ...). Esp. we already need special tricks to run kubectl commands, so it may actually simplify stuff...
tests/ctst/run-ctst-locally.sh
Outdated
There was a problem hiding this comment.
instead of passing specifically "tags", we could just forward"all" arguments to yarn, with "$@" : so the script is a "transparent" wrapper, and more flexible (if we need to pass extra cucumber params, for exemple to run a single test or multiple tags...)
may be a bit more tricky with the optional image though, may need to add more (bash) argument parsing, either to support running like this ./run-ctst-locally.sh [image] [-- <extra params>] or to use a flag ./run-ctst-locally.sh [-i <image>] <extra params>...
There was a problem hiding this comment.
I tried something but it got a little bit dirty with the parsing of the image indeed.
I did make a change so that we can now use natural Cucumber tags expression though
There was a problem hiding this comment.
just run.sh?
(since it may be used not just locally -we could/should use it in CI, to avoid having multiple code-path- and already in ctst folder)
There was a problem hiding this comment.
I strongly agree to avoid several .sh script. Also to make sure everything will continue to work, I think we should find a way to test that in the CI ?
There was a problem hiding this comment.
I'm not a big fan of run.sh, it's too short and grepping "run" returns thousands of results. Actually this is the reason i originally got confused, because in run-e2e-ctst, the pods was executing run, and it took me some time to realize it was the cli-testing run, and not this file. I'm not fixed on this specific name, but I think it's nice to have a more identifiable name, the current name, or "run-cucumber-tests.sh" for example leaves no room for ambiguity.
Also, more importantly, there is indeed an objective which would be that local tests and github ci tests are run using the same same file, but at the moment there isn't done, as there are still some difference between the 2 files. I think eventually we will do it, and when it happens we can think again about the naming
There was a problem hiding this comment.
My goal was mainly to avoid duplicate logic. I'm fine to iterate later, we should track that in a JIRA and in the epic and add some comments and the beginning of each 🙏
497a7db to
faea5c8
Compare
8ece2e3 to
b408f36
Compare
|
|
||
| # 2. Load into kind and reset the pod | ||
| kind load docker-image ctst-local:dev | ||
| kubectl delete pod ctst-end2end |
There was a problem hiding this comment.
why would it be created already?
is it created automatically on devcontainer startup?
There was a problem hiding this comment.
I updated the comment. Basically, only needed if the pod was already created before with a different image
| ARG DRCTL_TAG=latest | ||
|
|
||
| FROM ghcr.io/scality/sorbet:$SORBET_TAG AS sorbet | ||
| FROM ghcr.io/scality/zenko-drctl:$DRCTL_TAG AS drctl |
There was a problem hiding this comment.
note for later (more when we want to run CTST tests outside of k8s) : maybe we do not actually need these binaries, we could run them directly in pods...
(typically, esp. for Sorbet, this is often how it is done: kubectl exec into sorbetctl container...)
| format: [ | ||
| 'progress-bar', | ||
| '@cucumber/pretty-formatter', | ||
| 'json:reports/cucumber-report.json', |
There was a problem hiding this comment.
does this generate a JUnit report? (needed for the overall Zenko workflow, to report failed tests summary)
it is the "stock" reporter, or the one implemented in cli-testing ?
There was a problem hiding this comment.
okay I reviewed this last comment : my code changes do not modify the existing logic, in run-e2e-ctst, cucumber is ran with the same previous parameter (-- format junitxxxx) to generate the report.
What I checked though, is if this report was exported to the artifact, and I found it was not.
So I updated one of the action file to export the report to the artifact, and also found that the xml report is not really human readable, so I also exported an html version (while keeping the first format). Also bumped the action action-junit-report from v4 to v6
francoisferrand
left a comment
There was a problem hiding this comment.
LGTM except maybe for the https://github.com/scality/Zenko/pull/2319/changes#r2813543342 question : need to make sure we don't "break" failed tests reporting via JUnit test report
| COPY package.json ./ | ||
| COPY yarn.lock ./ | ||
|
|
||
| RUN --mount=type=secret,id=GIT_AUTH_TOKEN \ |
There was a problem hiding this comment.
nit: this will work in CI and codespace, but fail when running locally if SSH is used to auth with GitHub.
So (eventually, if we still use this image) we will need to further improve this to support both GIT_AUTH_TOKEN and SSH... e.g. something like adding the 2 mounts (if possible...), and replacing the URL depending on what we get...
2271c1f to
25586b6
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
25586b6 to
59e3b03
Compare
Issue: ZENKO-5180
59e3b03 to
90d69c5
Compare
DarkIsDude
left a comment
There was a problem hiding this comment.
For me we are good. Some small comments. If you have a re-review, just retrigger the flow. Thanks for your work !
|
|
||
| ```bash | ||
| # 1. Build the CTST image | ||
|
|
| ```bash | ||
| cd tests/ctst | ||
| ./run-ctst-locally.sh @getObject | ||
| ./run-ctst-locally.sh "@PreMerge and not @Flaky" # Use quotes for complex expressions |
There was a problem hiding this comment.
| ./run-ctst-locally.sh "@PreMerge and not @Flaky" # Use quotes for complex expressions | |
| ./run-ctst-locally.sh "@PreMerge and not @Flaky" |
You have the same letter. I think it's pretty a standard for bash command and it's not needed to explain it ?
There was a problem hiding this comment.
My goal was mainly to avoid duplicate logic. I'm fine to iterate later, we should track that in a JIRA and in the epic and add some comments and the beginning of each 🙏
ISSUE: ZENKO-5180
Run ctst inside Codespace :
Some other refactoring