Skip to content

ci: release from actions, only test changed files #374

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

Merged
merged 2 commits into from
Jun 6, 2025

Conversation

oopsbagel
Copy link
Collaborator

@oopsbagel oopsbagel commented Jun 6, 2025

This branch introduces release automation triggered by button clicks in Github Actions, guarded by a check on whether all the Cargo.toml files contain the same version string.

On PRs, changes to documentation no longer trigger code tests. Similarly, changes to code that don't update documentation do not trigger documentation tests. Changes that fail at the cargo check stage abort early to prevent lengthy CI builds of the installer and firmware.

Commits on the main branch always run the full test suite regardless of what changed.

Releases also run the full check, test, build and publish suite.

Pull Request Checklist

  • The Rayhunter team has recently expressed interest in reviewing a PR for this. If not, this PR may be closed due our limited resources and need to prioritize how we spend them.
  • Added or updated any documentation as needed to support the changes in this PR.
  • Code has been linted and run through cargo fmt
  • If any new functionality has been added, unit tests were also added

Notes

We can iterate on naming, functionality, etc. It's possible to break up the main file into smaller components (e.g., build.yml, mdbook.yml, etc, all triggered by main.yml conditionally). I've tested this a lot in my own repo, but it's always possible I've overlooked something.

Fixes #370

This commit introduces release automation triggered by button clicks in
Github Actions, guarded by a check on whether all the Cargo.toml files
contain the same version string.

On PRs, changes to documentation no longer trigger code tests.
Similarly, changes to code that don't update documentation do not
trigger documentation tests. Changes that fail at the `cargo check`
stage abort early to prevent lengthy CI builds of the installer and
firmware.

Commits on the `main` branch always run the full test suite regardless
of what changed.

Releases also run the full check, test, build and publish suite.
steps:
- uses: actions/checkout@v4
- name: Check formatting
run: cargo fmt --all --check
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a functionality change: now we enforce that the code has been run through cargo fmt.

If we accept this we can drop one of the "have you run a linter" PR checklists. I can do that in this PR if we like this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yesssss! I love this

- name: Ensure all Cargo.toml files have the same version defined.
run: |
defined_versions=$(find lib bin installer rootshell telcom-parser -name Cargo.toml -exec grep ^version {} \; | uniq | wc -l)
find lib bin installer rootshell telcom-parser -name Cargo.toml -exec grep ^version {} \;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is here to help with debugging actions when this guard fails, the versions will be printed to the actions log.

# https://github.com/EFForg/rayhunter/actions/workflows/release.yml
name: Release rayhunter
on:
workflow_dispatch:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could also trigger this on a v* tag push. My feeling is that starting with one canonical release mechanism is the right way though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I think having one release mechanism and documentation on how to trigger a release.

@oopsbagel
Copy link
Collaborator Author

The addition of all the permissions blocks will silence the CodeQL warnings: https://github.com/EFForg/rayhunter/security/code-scanning/19 and is good practice that we should keep.

@oopsbagel oopsbagel marked this pull request as ready for review June 6, 2025 00:13
@oopsbagel
Copy link
Collaborator Author

For future work, I'd like to:

  1. work with one of the admins to allowlist the actions/* and dtolnay/rust-toolchain actions as the only allowed actions on the repository.
  2. implement sbom generation in CI for releases
  3. include the offline documentation html bundle in the release zip
  4. use sigstore to generate an attestation for releases that people can check on rekor

Each of those should be pretty easy to implement individually.

@oopsbagel
Copy link
Collaborator Author

oopsbagel commented Jun 6, 2025

Replying to myself:

It's possible to break up the main file into smaller components (e.g., build.yml, mdbook.yml, etc, all triggered by main.yml conditionally).

I will probably do this in a separate PR later, I think it will make the workflows easier to read. If anyone wants to PR against my branch they can.

alliraine
alliraine previously approved these changes Jun 6, 2025
Copy link
Collaborator

@alliraine alliraine left a comment

Choose a reason for hiding this comment

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

Going to go ahead and drop an approval on here so you don't get to a state where you are blocked due to a lack of approvals.

Since this is a pretty major change to the release workflow we should probably wait until a few others chime in with their thoughts

untitaker
untitaker previously approved these changes Jun 6, 2025
@oopsbagel oopsbagel dismissed stale reviews from untitaker and alliraine via 9681970 June 6, 2025 18:26
@oopsbagel oopsbagel requested review from alliraine and untitaker June 6, 2025 18:27
@cooperq cooperq merged commit f536880 into EFForg:main Jun 6, 2025
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.

Enforce cargo fmt on codebase
4 participants