Add workflow with DTS tests #193
Conversation
Signed-off-by: Pawel Langowski <[email protected]>
|
The build step is short enough to just copy to the test workflow. e58279e#diff-faff1af3d8ff408964a57b2e475f69a6b7c7b71c9978cccc8f471798caac2c88R14-R41 |
|
And now if we change anything in this step we will have to remember to fix it in 3 places. We already had situations where we forgot to update second workflow. If you want to add yet another code duplication then this situation will likely happen again. |
Signed-off-by: Pawel Langowski <[email protected]>
|
Done |
|
Looks good, 2 comments left from previous PR, lock python version + somehow print test title + test status |
|
I specified the Python version cc9a438 and replied to the thread #189 (comment) |
|
Signed-off-by: Pawel Langowski <[email protected]>
Signed-off-by: Pawel Langowski <[email protected]>
PLangowski
force-pushed
the
tests-ci
branch
7 times, most recently
from
aa0ded1 to
1a5aa00
Compare
PLangowski
force-pushed
the
tests-ci
branch
4 times, most recently
from
897473c to
6aa3b23
Compare
|
@PLangowski Can you also use |
|
@PLangowski also update checkout action in build.yml? |
PLangowski
force-pushed
the
tests-ci
branch
2 times, most recently
from
8716e17 to
73a3a5f
Compare
Signed-off-by: Pawel Langowski <[email protected]>
|
I pushed my changes. @macpijan Let me know if you like this solution |
| shell: bash | ||
| run: | | ||
| mkdir ipxe | ||
| cp build/tmp/deploy/images/genericx86-64/dts-base-image-genericx86-64.cpio.gz ipxe |
There was a problem hiding this comment.
I think I would put this into the osfv/scripts/ci as well, just as qemu.
This way, you could replicate this more easily locally as well. I assume you might want to run these tests locally as well, prior creating PR.
There was a problem hiding this comment.
Move this script to osfv: d46ef63
PR to osfv: Dasharo/open-source-firmware-validation#620
There was a problem hiding this comment.
@PLangowski please check
There was a problem hiding this comment.
@PLangowski updated script in the osfv PR, tomorrow I will update its usage here
| shell: bash | ||
| id: log_dirs | ||
| run: | | ||
| timestamp=$(date -u +%Y-%m-%dT%H:%M:%S%Z) |
There was a problem hiding this comment.
./scripts/run.sh already does that for you
| ip_addr=$(ip -o -4 addr list eno2 | awk '{print $4}' | cut -d/ -f1) | ||
|
|
||
| robot -L TRACE -v config:qemu -v rte_ip:127.0.0.1 -v snipeit:no \ | ||
| -v dpp_password:$DPP_PASSWORD -v dpp_download_key:$MSI_DOWNLOAD \ |
There was a problem hiding this comment.
I guess you could shorten execution in CI by using https://robotframework.org/robotframework/latest/libraries/OperatingSystem.html#Get%20Environment%20Variable in tests to retrieve credentials, if we assume to always get these from OS environment
| source ~/.secrets/dpp-keys | ||
| ip_addr=$(ip -o -4 addr list eno2 | awk '{print $4}' | cut -d/ -f1) | ||
|
|
||
| robot -L TRACE -v config:qemu -v rte_ip:127.0.0.1 -v snipeit:no \ |
There was a problem hiding this comment.
Is there a reason we do not use https://github.com/Dasharo/open-source-firmware-validation/blob/develop/scripts/run.sh here? We do not want to publish full logs?
But we do not store artifacts anyway?
Please explain it here.
There was a problem hiding this comment.
We don't want to print full logs because in case of a failure they might leak DPP credentials
There was a problem hiding this comment.
If running these DTS scripts is very different than running ./run.sh script on the DTS suite, we may consider adding regression script for DTS as well, so it is easier to run it locally, as well as to have fewer changes in this CI configuration file.
There was a problem hiding this comment.
But maybe it is not (or does not have to be) and we can simply run the run.sh script?
There was a problem hiding this comment.
We don't want to print full logs because in case of a failure they might leak DPP credentials
To the terminal console?
Do we log the DPP variables to the console?
I know we might have them in HTML log files, but can we have them in the console output as well?
There was a problem hiding this comment.
We can use https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/workflow-commands-for-github-actions#masking-a-value-in-a-log to lessen chance that secret information gets logged.
Though I'm not sure if build logs are visible to anyone?
| -v dts_ipxe_link:http://${ip_addr}:4321/dts.ipxe | ||
| -i "msi" dts/dts-e2e.robot 2>&1 | tee $LOG_DIR/output_msi.log | grep "| PASS |\|| FAIL |" | ||
|
|
||
| robot -L TRACE -v config:qemu -v rte_ip:127.0.0.1 -v snipeit:no \ |
There was a problem hiding this comment.
This seems overly complex and scales poorly. Again, what happens if we add another platform/test in the dts-e2e.robot? We need to update the CI file as well - this is not optimal.
Why cannot we run the whole suite in one run?
We can modify the tests in the suite to set DPP variables in each test case accordingly, based on the environment variables.
There was a problem hiding this comment.
There was a problem hiding this comment.
This will be a problem for a short period of time, because soon, when we migrate our infrastructure to MinIO, we will be able to use one set of credentials for all subscriptions. So, I would rather stay with the way it is now.
| imgfetch --name file_initrd dts-base-image-genericx86-64.cpio.gz\n | ||
| kernel file_kernel root=/dev/nfs initrd=file_initrd\n | ||
| boot" > ipxe/dts.ipxe | ||
| cd ipxe && python3-m http.server 4321 & |
There was a problem hiding this comment.
Are we certain all of these background processes are always killed when workflow is interrupted, or we need to take care of this?
This is less of a problem on GitHub runners, as each time new VM is started, but on the self-hosted runner, we may end up with zombie processes if this is not the case.
|
@macpijan The current solution does work. Your points are solid (especially the one with the env vars), but we need this workflow in today's release, so maybe we should implement them later? What do you think? |
We can do that, just create an issue - can be a single one linking to this MR. |
Signed-off-by: Pawel Langowski <[email protected]>
No description provided.