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 action workflow check that enforces 'cargo fmt' #97

Closed
wants to merge 2 commits into from

Conversation

Nukesor
Copy link
Contributor

@Nukesor Nukesor commented Oct 3, 2020

Hey there :)

This PR adds a cargo fmt check to the rust Github action.
That should enforce/remind people to use cargo fmt in their PRs.

I'm also thinking about adding another PR, which resolves all open cargo clippy issues and adds a cargo clippy check to the action. If you don't mind, I'll start working on that as well momentarily.

This also adds a rule, which actually runs the checks on PRs.

@Nukesor Nukesor force-pushed the cargo-fmt-workflow branch from a3e6004 to 688a491 Compare October 3, 2020 15:12
@Nukesor
Copy link
Contributor Author

Nukesor commented Oct 3, 2020

I'm not exactly sure why this test is failing. This doesn't look related to any changes I made. The other run in the PR that builds upon this one looks like it's working.

Maybe you have some non-deterministically failing tests?

@eminence
Copy link
Owner

eminence commented Oct 3, 2020

Yeah, some of the tests are a little racey (a few milliseconds go by between when we get a process listing and when we try to get info about it -- in that time, the process might have exited). I've re-run the tests and they are passing now.

This same comment applies to both of your pull requests: I don't really have an objection to adding clippy or rustfmt to CI (as you'll see several commits in the git history were I apply clippy or fmt fixes), but I'm a little hesitant to block PRs on them. It's a bit of a barrier/annoyance that I'm OK dealing with myself, but I don't necessarily want all contributors to deal with it if they don't want to.

Can we try to mark the rustfmt and clippy jobs as "continue-on-error" (and then introduce a "fake" clippy or fmt error to see what it looks like)? I've not used this github action setting before, but I'm hoping it might be like gitlab's "allow-failure" where it turns the job a yellow color.

@Nukesor
Copy link
Contributor Author

Nukesor commented Oct 4, 2020

I looked around and those configs aren't an option yet.

This allows workflows to not fail when some specific steps fail, but there's no indication that anything went wrong.
actions/runner#2347

A workaround would be to set the outcome status manually, but this isn't possible yet either -.-
actions/runner#662

I'll remove the checks and simply fix all formatting and clippy issues, plus add clippy exceptions for stuff that would break backward compatibility

@eminence
Copy link
Owner

eminence commented Oct 4, 2020

Thanks for researching this a bit. It's a bummer that github doesn't support this, though hopefully one day they will! Something like Gitlab's "allow_failure: true" would fit well here, I think.

So regarding these two pull requests, let's close them, since github actions can't really do what we want.

If you wanted to open another PR with some clippy/fmt fixes, I'd be happy to accept that :)

@Nukesor Nukesor closed this Oct 4, 2020
@Nukesor Nukesor deleted the cargo-fmt-workflow branch October 4, 2020 11:49
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.

2 participants