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

CI in OQS: guidelines for responsible use #5

Open
bhess opened this issue Mar 20, 2024 · 10 comments
Open

CI in OQS: guidelines for responsible use #5

bhess opened this issue Mar 20, 2024 · 10 comments
Assignees
Labels
Low priority Could be dealt with at a later time

Comments

@bhess
Copy link
Member

bhess commented Mar 20, 2024

This issue is to suggest setting guidelines in OQS for resource-responsible use of CI.

OQS so far already did much more than "typical projects" in this regards/not blindly running CI in the various projects -- but there's still quite some way to go: What about OQS "institutes" this? Ask contributors to use "[skip ci]" as often as possible, do (CI) spot checks before doing "heavy lifting", possibly move more CI to the weekly tests, reduce the repetition of the profiling runs, etc?

Originally posted by @baentsch in open-quantum-safe/oqs-provider#376 (comment)

I agree with the above and think some guidelines would help to avoid redundant CI runs, without hurting the overall test quality. This is not necessarily motivated by a lack of available resources (the LF indicated that more Github Actions resources can be made available to the projects, which I think is a good thing), but rather by the desire to make most efficient use of the available resources.

Any feedback welcome.

@ghost
Copy link

ghost commented Mar 20, 2024

I think this is a good idea.

One thing we could do to pave the way is to smartly list all files that require tests to be run (basically *.c, *.h, and some generated .md files?). If the changes don't involve at least one of these files, then nothing would be run.

@SWilson4
Copy link
Member

SWilson4 commented Apr 1, 2024

I agree that this is a good idea. One "obvious" way to make CI more efficient would be to eliminate redundant runs (e.g., the same workflow being triggered by both "push" and "pull_request" on some PRs). I haven't yet found a clean way to do this, but perhaps somebody with more GitHub-specific expertise might.

IMO this is a great place to contribute for those who want to help out OQS / PQCA but don't feel comfortable working with cryptographic code.

@dstebila
Copy link
Member

dstebila commented Apr 1, 2024

I agree that this is a good idea. One "obvious" way to make CI more efficient would be to eliminate redundant runs (e.g., the same workflow being triggered by both "push" and "pull_request" on some PRs). I haven't yet found a clean way to do this, but perhaps somebody with more GitHub-specific expertise might.

@ryjones might have experience with this.

@ryjones
Copy link
Contributor

ryjones commented Apr 1, 2024

@SWilson4 do you mean on PR and merge, or the same job runs on both PR and the push of the PR to a branch?

@SWilson4
Copy link
Member

SWilson4 commented Apr 1, 2024

@SWilson4 do you mean on PR and merge, or the same job runs on both PR and the push of the PR to a branch?

The latter. For example, see the "Linux and macOS tests" here: https://github.com/open-quantum-safe/liboqs/pull/1725/checks. These tests (and all others triggered to run on both push and pull_request events run twice on every push to an existing PR (at least for my PRs and presumably non-fork PRs).

@ryjones
Copy link
Contributor

ryjones commented Apr 1, 2024

@SWilson4 this stack overflow answer shows the way.

  push:
    branches:
      - 'main'
      - 'releases/**'

might be better

@ryjones
Copy link
Contributor

ryjones commented Apr 1, 2024

take a look here

@dstebila dstebila assigned bhess and ghost Apr 2, 2024
@dstebila
Copy link
Member

dstebila commented Apr 2, 2024

The outcome of today's OQS status call was that it would be good to have a document setting some guidelines / documenting best practices, saved as a Markdown file in the TSC repository. @bhess and @thb-sb to kick it off.

@baentsch
Copy link
Member

baentsch commented Apr 2, 2024

Thanks very much for documenting decisions such as this, @dstebila .

May I augment with the suggestion to link the file thus generated from a (new) "getting started contributing to OQS" write-up at www.openquantumsafe.org? Maybe off/in/at https://openquantumsafe.org/about/#getting-started (?)

@SWilson4
Copy link
Member

SWilson4 commented Sep 4, 2024

In the absence of codified guidelines, here are some principles I tried to follow in open-quantum-safe/liboqs#1909. I would suggest that we work toward following these in other projects as well.

  1. Cancel in-progress jobs on a double-push. This can be achieved using concurrency groups.
  2. Only run resource-heavy tests if basic checks pass first.
  3. Run only a minimal set of tests automatically on push to non-main branches.
  4. As a follow-up to the above, give developers the ability to trigger heavier tests manually—ideally both by pushing a button in the GitHub UI (using workflow dispatches) and by including a keyword in the commit message.

I found reusable workflows to be very helpful for implementing all of these. These allow us to create "caller" workflows for each condition (push, pull request, release, schedule, etc) which trigger a subset of separate "callee" workflows (macOS tests, Linux tests, downstream tests, extended tests, etc). With this pattern we can get very fine-grained control over which tests run and when.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Low priority Could be dealt with at a later time
Projects
None yet
Development

No branches or pull requests

5 participants