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

Add CI workflow checking #1880

Closed
wants to merge 4 commits into from
Closed

Conversation

jplomas
Copy link
Contributor

@jplomas jplomas commented Aug 3, 2024

This PR adds an Actionlint workflow (and an associated config file to define the custom runner) to validate GH actions as per #1866

This would, for example, have errored with the workflow issues fixed in #1869

* [ ] 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.)

jplomas added 3 commits August 3, 2024 23:08
Signed-off-by: JP Lomas <jp@theqrl.org>
Signed-off-by: JP Lomas <jp@theqrl.org>
Signed-off-by: JP Lomas <jp@theqrl.org>
@jplomas
Copy link
Contributor Author

jplomas commented Aug 6, 2024

Not sure why this Travis job has errored - looks like a runner issue. Perhaps someone with appropriate access could trigger a build manually?

@SWilson4
Copy link
Member

SWilson4 commented Aug 6, 2024

Not sure why this Travis job has errored - looks like a runner issue. Perhaps someone with appropriate access could trigger a build manually?

Done. It does indeed look like a runner issue; our Travis setup is somewhat unreliable.

@jplomas
Copy link
Contributor Author

jplomas commented Aug 6, 2024

Thank you. Looks like it is still timing out. Is it worth asking support@travis.com if they can take a look? This is the suggested course of action in their docs...

@SWilson4
Copy link
Member

SWilson4 commented Aug 7, 2024

Thank you. Looks like it is still timing out. Is it worth asking support@travis.com if they can take a look? This is the suggested course of action in their docs...

I think @bhess maintains our Travis setup: Basil, could you please take a look at the recent build failures?


- name: Install Actionlint
run: |
curl -sSL https://github.com/rhysd/actionlint/releases/download/v1.7.1/actionlint_1.7.1_linux_amd64.tar.gz | tar -xz -C /usr/local/bin
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if there's a way to do this so that we're not excecuting a file downloaded from a somewhat random URL. Perhaps we could

  • use a (somewhat) trusted method like apt-get (if supported),
  • host the archive somewhere on a URL we control so that we know it won't change silently, or
  • run this in one of our CI containers and bake actionlint into the image.

Tagging @planetf1 for ideas here as this is adjacent to the scorecard work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could also build Actionlint from source at a fixed commit in this Action

Copy link
Member

Choose a reason for hiding this comment

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

We could also build Actionlint from source at a fixed commit in this Action

Quick reminder: We want to be a bit more responsible to the environment, so building things at each CI run seems to run counter to this goal. Also, for more reliability, I'd think this option by @SWilson4 is the most sensible way forward:

run this in one of our CI containers and bake actionlint into the image.

@@ -0,0 +1,7 @@
self-hosted-runner:
Copy link
Member

Choose a reason for hiding this comment

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

Technically this isn't a self-hosted runner: it's a GitHub-hosted runner that we've configured. I'm not sure if that makes a difference in the config.

Copy link
Contributor Author

@jplomas jplomas Aug 21, 2024

Choose a reason for hiding this comment

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

Without this, the lint will fail with the error:

.github/workflows/unix.yml:86:21: label "oqs-arm64" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-14.0", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-13.0", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "macos-12.0", "macos-11", "macos-11.0", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file [runner-label]

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. It looks like there's no other name for the actionlint config variable, but maybe we can add a comment saying that it's not self-hosted. A security audit asked about the oqs-arm64 being self-hosted, so it would be nice to not have any documentation that might make it appear to be.

@SWilson4
Copy link
Member

Given that we've set a precedent of ignoring Travis failures until #1888 is resolved, I suggest that we do the same here and try to move this forward.

@baentsch
Copy link
Member

Given that we've set a precedent of ignoring Travis failures until #1888 is resolved, I suggest that we do the same here and try to move this forward.

Not really. It's a bad precedent and resolution should be along these lines; Thus @jplomas can I ask you to rebase this PR (as Travis has been disabled for now) and add commentary as per @SWilson4 's comment above and into CONTRIBUTING.md stating that no PR shall (be allowed to) proceed unless this specific lint job passes (thus protecting the CI chain and against wrong approval decisions based on improper CI runs)?

@jplomas
Copy link
Contributor Author

jplomas commented Aug 30, 2024

This will work well in the in-progress basic.yml workflow in #1909, so I will close this PR and make a new one (including suggested revisions) when #1909 complete. In the mean time, I'll add actionlint into the ubuntu-latest CI container.

@jplomas jplomas closed this Aug 30, 2024
@jplomas jplomas deleted the main branch August 31, 2024 16:16
jplomas added a commit to jplomas/liboqs that referenced this pull request Sep 10, 2024
This PR adds an Actionlint workflow to validate GH actions as per open-quantum-safe#1866

This is an updated version of PR open-quantum-safe#1880, taking into account the discussion on that contribution.

Signed-off-by: JP Lomas <jp@theqrl.org>
SWilson4 added a commit that referenced this pull request Sep 11, 2024
* Check workflows for issues during CI

This PR adds an Actionlint workflow to validate GH actions as per #1866

This is an updated version of PR #1880, taking into account the discussion on that contribution.

Signed-off-by: JP Lomas <jp@theqrl.org>

* CONTRIBUTING.md update

Documents actionlint use as part of CI basic workflow including instructions of running locally.

Signed-off-by: JP Lomas <jp@theqrl.org>

* Update .github/workflows/basic.yml

Co-authored-by: Spencer Wilson <spencer.wilson@uwaterloo.ca>
Signed-off-by: JP Lomas <jp.lomas@gmail.com>

---------

Signed-off-by: JP Lomas <jp@theqrl.org>
Signed-off-by: JP Lomas <jp.lomas@gmail.com>
Co-authored-by: Spencer Wilson <spencer.wilson@uwaterloo.ca>
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.

3 participants