-
Notifications
You must be signed in to change notification settings - Fork 134
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
Add GH actions to CI #383
Add GH actions to CI #383
Conversation
This Patch: - Adds Github Actions to CI - reuses scripts under ./travis/ (requires improvement) - Uploads artifacts to GH by removing dependency on transfer.sh - Uploads built rpms (auto-deleted every 90 days) - Runs build job on container provisioned by GH actions (ie) not necessary to meddle with docker/podman commands manually - Run PKI test job on self-provisioned container (room for improvement by running on service containers by combining with Docker GH actions) Signed-off-by: Dinesh Prasanth M K <dmoluguw@redhat.com>
Hmm. The new GH actions CI doesn't run in this repo until merged. But, you can browse through the CI job that ran in my personal repo: https://github.com/SilleBille/pki/actions/runs/84282813 |
I don't think we're really worried about that right now -- we're running on upstream (Azure hosted) hardware with this PR right? |
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 think it looks good, but please see my comments/questions below. Is there anything that we need to set up in order to run the GH actions in our own repo before submitting the PR? If so, could you please provide a link to the instruction? Thanks!
# run: docker exec -i ${CONTAINER} dnf copr enable -y ${COPR_REPO} | ||
|
||
- name: Install DS | ||
run: docker exec -i ${CONTAINER} ${PKIDIR}/travis/ds-create.sh |
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.
Eventually we want to run the test scripts from pki-tests package that we just built instead of from the source repository, but this is fine for now.
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. Currently, I used the existing scripts. Since the artifacts are shared, it is possible to use the tests from the pki-tests
package
run: docker exec -i ${CONTAINER} bash -c "journalctl -u pki-tomcatd@pkitest > /var/log/pki/pki-journalctl.log" | ||
|
||
- name: Compress logs | ||
if: always() |
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.
Just curious, what is the purpose of if: always()
?
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.
always()
makes sure that this step gets executed even if any of the previous step fails. By default, all the steps after a failed step are skipped. In future, we can change this to failure()
which will run only when one of the previous step fails. More information
run: docker exec -i ${CONTAINER} ${PKIDIR}/travis/ds-create.sh | ||
|
||
- name: Install CA | ||
run: docker exec -i ${CONTAINER} ${PKIDIR}/travis/ca-create.sh |
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.
If the installation fails we have code in ca-create.sh
to upload the logs to transfer.sh for troubleshooting. Can that code be changed to upload to GH instead eventually?
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. That's possible and easier too.
Since this patch uploads all the logs in /var/log/pki
, we can do a minor change in future to [subsystem]-create.sh
:
SYSTEMD_LOG=/var/log/pki/pkitest-systemd-[subsystem].log
FWIW, this patch includes PKI journalctl logs as an artifact
PKIDIR: /tmp/workdir/pki | ||
LOGS: ${GITHUB_WORKSPACE}/logs.txt | ||
COPR_REPO: "@pki/master" | ||
test_set: "test_caacl_plugin.py test_caacl_profile_enforcement.py test_cert_plugin.py test_certprofile_plugin.py test_ca_plugin.py test_vault_plugin.py" |
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.
Since the build is now only done once, maybe we can split some of the IPA tests to run concurrently to reduce the execution time.
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, that's possible too \o/ May be in a future PR?
For the tests after installation can we verify if the services ca, kra, etc are running? |
@cipherboy yes, this patch runs on Github provided runners
@bhavikbhavsar This patch does the same thing as we do in Travis (in fact, uses the same scripts). Going forward, I'll be working with @06shalini to run actual test playbooks. |
This commit: - Adds IPA tests to the GH actions matrix - Gathers logs of IPA and PKI and corresponding journalctl logs and uploads it as a GH action artifact - Minor cleanups to previous jobs Signed-off-by: Dinesh Prasanth M K <dmoluguw@redhat.com>
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
This patch adds GH actions as part of the CI. This patch reuses scripts from
travis/
Pros
build -> test
(ie) test only if build passes, unlike Travis where we execute same scripts in every job.master
andgh-actions
(will be removed in future) branchesdocs/
can be ignored.Cons
.yaml
fileFuture improvements
lint -> unit test -> build -> functional test