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 github actions CI and remove travis CI #89

Merged
merged 6 commits into from
May 29, 2024
Merged

Add github actions CI and remove travis CI #89

merged 6 commits into from
May 29, 2024

Conversation

faern
Copy link
Member

@faern faern commented May 3, 2024

The CI for this crate has not been running for a looong time. We have not used Travis for years.

This PR:

  • Removes the defunct Travis CI
  • Adds github CI that checks for:
    • Rust formatting
    • Audit dependencies
    • Build and run tests
    • Builds with minimal dependencies
    • Git commit message style
  • Checks in the lockfile for a more consistent CI experience

I wish we could also run cargo clippy. Hopefully we can in the future. But currently clippy complains about so much that I leave it as a separate exercise.


This change is Reviewable

@pronebird
Copy link
Contributor

pronebird commented May 3, 2024

Note that running tests on macOS require root, since we access /usr/bin/pf. Alternatively we could feature-gate integration tests, but of course it would be beneficial to have them running as a part of CI.

@faern
Copy link
Member Author

faern commented May 3, 2024

In this case the tests can't even be built. Look at the errors. My guess is that pfvar.rs must be re-generated with a newer version of bindgen or something.

@pronebird
Copy link
Contributor

pronebird commented May 3, 2024

In this case the tests can't even be built. Look at the errors. My guess is that pfvar.rs must be re-generated with a newer version of bindgen or something.

Yes either rebuild bindings with new bindgen or pick it up from #87 where I had also updated pfvar.h with the most recent.

@faern faern force-pushed the add-ci branch 2 times, most recently from 05fa368 to 44c2eef Compare May 28, 2024 14:30
@faern faern force-pushed the add-ci branch 5 times, most recently from b481d49 to 8e9553f Compare May 29, 2024 08:56
@faern faern marked this pull request as ready for review May 29, 2024 08:59
@faern faern force-pushed the add-ci branch 2 times, most recently from 8ae46dc to 2578f19 Compare May 29, 2024 09:37
@faern faern requested a review from Serock3 May 29, 2024 09:39
Copy link

@Serock3 Serock3 left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 7 files at r1, 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @faern)


.github/workflows/build-and-test.yml line 37 at r4 (raw file):

        # Since the tests modify global state (the system firewall) they cannot run in parallel.
        # The integration tests must run as root since they write firewall state (/dev/pf)
        run: sudo cargo test -- --test-threads=1

The output of the action seems to indicate that this command causes a rebuild, which will then be done with root access


.github/workflows/cargo-audit.yml line 35 at r4 (raw file):

          # Ignored audit issues. This list should be kept short, and effort should be
          # put into removing items from the list.
          ignore:

The ignore section is empty, should it still be there?


.github/workflows/formatting.yml line 5 at r4 (raw file):

on:
  pull_request:
    paths:

Should rustfmt.toml not be included here?

Copy link
Member Author

@faern faern left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Serock3)


.github/workflows/cargo-audit.yml line 35 at r4 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

The ignore section is empty, should it still be there?

I thought it was better to keep it. Since it makes it easier to add when needed. Most projects will oscillate between zero and ~1-2 ignored issues. And if they every time they go from zero to non-zero have to add the boilerplate including the docs, that's more churn. I figured it's nicer to keep the docs and keyword even when empty. Do you agree?


.github/workflows/formatting.yml line 5 at r4 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Should rustfmt.toml not be included here?

I was hoping you would not see that one. Because I aim to remove that settings file but I wanted to keep it to a separate PR to not scope creep.

Copy link
Member Author

@faern faern left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Serock3)


.github/workflows/formatting.yml line 5 at r4 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I was hoping you would not see that one. Because I aim to remove that settings file but I wanted to keep it to a separate PR to not scope creep.

#95

Copy link

@Serock3 Serock3 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @faern)


.github/workflows/cargo-audit.yml line 35 at r4 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I thought it was better to keep it. Since it makes it easier to add when needed. Most projects will oscillate between zero and ~1-2 ignored issues. And if they every time they go from zero to non-zero have to add the boilerplate including the docs, that's more churn. I figured it's nicer to keep the docs and keyword even when empty. Do you agree?

Alright


.github/workflows/formatting.yml line 5 at r4 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I was hoping you would not see that one. Because I aim to remove that settings file but I wanted to keep it to a separate PR to not scope creep.

Haha I see, let's ignore it for now then.

Copy link
Member Author

@faern faern left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Serock3)


.github/workflows/build-and-test.yml line 37 at r4 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

The output of the action seems to indicate that this command causes a rebuild, which will then be done with root access

Yes. But as I wrote on slack (repeating just for documentation):

  • Building the entire thing takes ~10 seconds. So optimizing is hardly worth it
  • There are no secrets on this build machine/container, so it does not matter if its compromized
  • The container has passwordless sudo anyway. So it would be trivial to compromise even if we did not use sudo ourselves.

Copy link

@Serock3 Serock3 left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@faern faern merged commit a71988b into main May 29, 2024
9 checks passed
@faern faern deleted the add-ci branch May 29, 2024 14:09
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