Skip to content
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

Move from CircleCI to GitHub Actions #1849

Merged
merged 9 commits into from
Jul 24, 2024
Merged

Move from CircleCI to GitHub Actions #1849

merged 9 commits into from
Jul 24, 2024

Conversation

SWilson4
Copy link
Member

This PR moves our remaining CircleCI workflows over to GitHub Actions. I've left it in draft for now as it's based on the branch for #1848.

I have tried to duplicate the testing coverage from CircleCI without additions or removals. This PR isn't intended to overhaul or evaluate our test coverage, it's just meant to move it to a different platform.

Three items of note:

(1) The CCI bionic-i386 could not be ported over, as the GitHub checkout action requires a newer version of node and can't run in the older container (at least not without some finicky work). Bionic is no longer supported anyhow, so it's probably for the best that we're no longer using it for testing. However, the multiarch images that we'd been using don't have options for i386 past Bionic, so there is no easy way to continue testing on i386. We don't list Linux / x86 as a supported platform, so I've opted to simply remove CI support for it.

(2) The CCI focal-clang-15 job had -DOQS_OPT_TARGET=skylake set. It didn't configure -DOQS_DIST_BUILD=OFF, however, so the opt target value was ignored. I have omitted the opt target variable in GitHub CI to reflect what was actually being tested.

(3) Finally, @ryjones helped me add a CircleCI API token to the liboqs environment so that GitHub can trigger CircleCI for downstream projects. These triggers are contained in the new commit-to-main.yml file. We may want to revise them for completeness, but for now I have left them as they were on CCI.

Fixes #1795.

  • Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
  • Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in fully supported downstream projects dependent on these, i.e., oqs-provider will also need to be ready for review and merge by the time this is merged.)

@SWilson4 SWilson4 marked this pull request as ready for review July 21, 2024 13:59
--user ${BUILD_TRIGGER_TOKEN}: \
--request POST \
--header "Content-Type: application/json" \
--data '{ "branch": "OQS-OpenSSL_1_1_1-stable", "parameters": { "run_downstream_tests": true } }' \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't look necessary to keep this one

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as below re liboqs-rust applies here.

--user ${BUILD_TRIGGER_TOKEN}: \
--request POST \
--header "Content-Type: application/json" \
--data '{ "branch": "OQS-v8", "parameters": { "run_downstream_tests": true } }' \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When to add/change this to v9?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The v9 branch doesn't yet work; the current PR from @geedo0 gets the basic test suite running but still doesn't have full PQ or hybrid support. I think we can merge this PR with v8, and then when v9 is ready, we can come back and update the liboqs CI.

--request POST \
--header "Content-Type: application/json" \
--data '{ "branch": "master" }' \
https://circleci.com/api/v2/project/gh/open-quantum-safe/liboqs-java/pipeline | tee curl_out \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't look as necessary as triggering liboqs-rust, does it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or liboqs-go... My intent with this PR was just to duplicate existing CCI functionality as closely as possible to make sure it worked on GH and make any improvements in a follow-up PR. But I could do both in one shot, if that would be preferable from a review standpoint.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@baentsch I'll interpret the thumbs-up as an endorsement of a follow-up PR to address #1783 and merge this as is.

.github/workflows/unix.yml Show resolved Hide resolved
Signed-off-by: Spencer Wilson <[email protected]>
Signed-off-by: Spencer Wilson <[email protected]>
Signed-off-by: Spencer Wilson <[email protected]>
Signed-off-by: Spencer Wilson <[email protected]>
Signed-off-by: Spencer Wilson <[email protected]>
@SWilson4 SWilson4 merged commit 2f02bf4 into main Jul 24, 2024
117 of 118 checks passed
@SWilson4 SWilson4 deleted the sw-cci-gha-move branch July 24, 2024 17:08
@SWilson4
Copy link
Member Author

The downstream trigger failed after the merge. I can trigger downstream pipelines locally with the same API token that should be in CI, so I think this is a GitHub variables config error. Will follow up with @ryjones later today.

rtjk pushed a commit to rtjk/liboqs that referenced this pull request Aug 5, 2024
Duplicate jobs from the CircleCI workflow as closely as possible in GitHub Actions. Remove Ubuntu Bionic / i386 support in CI.

---------

Signed-off-by: Spencer Wilson <[email protected]>
Signed-off-by: rtjk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move from CCI to GH CI
3 participants