-
Notifications
You must be signed in to change notification settings - Fork 15
Gh act improvements #137
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
base: main
Are you sure you want to change the base?
Gh act improvements #137
Conversation
|
Still in progress, one more thing left to do. |
21c0641 to
186770a
Compare
|
maybe split in multiple PRs? like cijoe version update separate ? |
Sure; doesn't really make much difference to me. |
Add a simple Dockerfile to enable users building a local image which includes Github CLI tool. With this image more workflows can be run locally without the need to temporarily modify the workflows just to install "gh". Signed-off-by: Karol Latecki <karol.latecki@nutanix.com>
Installing nodejs into the images fixes running Github Action workflows locally when using "gh act". Specifically: the parts of the workflows where "container:" is used with Fedora and Freebsd Docker images. Without nodejs available some of the actions (for example - actions/download-artifacts) will not work. Signed-off-by: Karol Latecki <karol.latecki@nutanix.com>
The version with outer double quotes and single inner quotes
works in Github using github-hosted runners, but not locally
using "gh act". "artifact_id" was actually getting exported
as:
'artifact_id={'\''fedora_43'\'':20878294770,'\''freebsd_14'\'':20878294770}'
Gh act JSON parser complains about single quotes and escape
signs, resulting in expressions being evaluated to NIL.
Use double quote for keys in JSON, which fixes the issue for gh act.
Signed-off-by: Karol Latecki <karol.latecki@nutanix.com>
The condition was wrong resulting in shutdown being attepmted for container-based workload in local (gh act) environment. We want to do this only on github-hosted runners and vm-based workloads. Signed-off-by: Karol Latecki <karol.latecki@nutanix.com>
Clear out github.token in case workflow is run locally using "gh act". Using the token results in the artifact being downloaded from Github __every time__ instead of using artifacts cached locally by act (and that's even if offline mode or artifact server path are used). Signed-off-by: Karol Latecki <karol.latecki@nutanix.com>
Use a dynamic value for disk.path based on a variable. The path will be different when running the job using Github-hosted runners and locally using "gh act". Signed-off-by: Karol Latecki <karol.latecki@nutanix.com>
186770a to
1d2bb32
Compare
|
@glimchb removed this particular commit from the PR |
| run: | | ||
| dnf update -y | ||
| dnf install -y git perl-JSON-PP | ||
| dnf install -y git perl-JSON-PP nodejs |
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.
Its not clear to me why this change is needed in the SPDK Fedora 43 image. Actions like download-artifacts are performed on cijoe (VM jobs) or SPDK Fedora 43 (Container jobs) containers, correct ?
Both of those container images stay the same whether we execute in GH or via ACT. Why is it not required currently in GH workflows we have ?
| - name: guest_update | ||
| run: | | ||
| pkg install -y git | ||
| pkg install -y git nodejs |
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.
Even more so than above. The FreeBSD VM images are not used as container images for executing GH actions.
|
|
||
| - name: qemu-guest, shutdown | ||
| if: ${{ contains(matrix.workflow.name, 'vm') && contains(runner.labels, 'self-hosted') || env.ACT == 'true' }} | ||
| if: contains(matrix.workflow.name, 'vm') && contains(runner.labels,'self-hosted') && ! contains(github.actor, 'nektos/act') |
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.
Right now it does NOT execute on github-hosted runners. My understanding was that it is exactly what we want - guest_shutdown is not necessary if the whole runner is being taken down shortly after the job ends. The commit message seems to say the oposite.
Doesn't GH ACT present itself as github-hosted runner ? If so then following check should be enough:
if: contains(matrix.workflow.name, 'vm') && contains(runner.labels,'self-hosted')
|
|
||
| - name: qemu-guest, shutdown | ||
| if: ${{ contains(matrix.workflow.name, 'vm') && contains(runner.labels, 'self-hosted') || env.ACT == 'true' }} | ||
| if: contains(matrix.workflow.name, 'vm') && contains(runner.labels,'self-hosted') && ! contains(github.actor, 'nektos/act') |
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've noticed there are two other similar checks for Cleanup runner step. It seems to work fine, just that the step executing and immediately exiting is strange. Considering changes in this PR, could you check if it is possible to execute the step the same way as here ?
| with: | ||
| name: vm-image-${{matrix.distro}}_x86_64 | ||
| github-token: ${{ github.token }} | ||
| github-token: ${{ ! contains(github.actor, 'nektos/act') && github.token || '' }} |
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.
How does cache and download actions work with ACT ? Ideally the download step would detect the qcow2 already being present with hashFiles.
Avoiding passing the token seems like a round-about way to skip this step, especially if one does not already have the image.
Series of changes improving running workflows locally using
gh act.Explanations here to avoid going into commit messages:
artifact_idin common tests workflow - there was an issue with quoting whichgh actJSON parser was complaining about. Interestingly - this worked in Github which might suggest different JSON parsers?gh actdisk.pathproblem of .toml file, but decided to keep the version upgrade anyway.