A better story for canary jobs (yellow CI) #33
Conversation
Enabling _all_ clippy warnings is too hardcore
They're not really smoke tests since they don't pre-empt other tests from running. Also, canaries are yellow, just like your CI will be. This also turns cargo +beta test into an allow_fail, because it is also a canary.
jonhoo
changed the title
| ${{ if ne(parameters.err_on_warn, 'true') }}: | ||
| displayName: cargo check | ||
| ${{ if ne(parameters.rust, 'stable') }}: | ||
| ${{ if eq(parameters.err_on_warn, 'true') }}: | ||
| displayName: "env RUSTFLAGS=-Dwarnings cargo +${{ parameters.rust }} check" | ||
| displayName: "cargo +${{ parameters.rust }} check -Dwarnings" |
There was a problem hiding this comment.
Should we allow deprecated? It'll make things more stable.
There was a problem hiding this comment.
This is already allow_fail, so I'd be inclined to still give deprecation warnings. They're things you should probably fix, just like other warnings.
There was a problem hiding this comment.
- This template does not have an
allow_fail - This assumes that the caller will always pass
allow_failwitherr_on_warn. In my use case, I do not want to be doing that.
There was a problem hiding this comment.
Ah, good catch! Fixed 1 in latest commit.
As for 2, it doesn't really assume that. If you specify err_on_warn without allow_fail, you are specifically saying "I want to treat warnings as CI errors". Seems reasonable for that to be a valid option?
There was a problem hiding this comment.
While so far I've gotten away with deprecated being enabled, you've brought up the concern over dependencies breaking people. If we want to take that seriously and support people who want a pinned warnings check, then we'd need to disable that warning.
There was a problem hiding this comment.
My thinking here was that either you care about warnings, or you do not (this is err_on_warn). If you do, you either want them to cause red CI or you want them to cause yellow CI (this is allow_fail). We could add a third branch point which is "care about all but warnings caused by changes in dependencies" I suppose (what would it be called)? My guess is that that option is less common, but I have limited data on that.
There was a problem hiding this comment.
My care abouts
- Warnings make CI red
- Transient state (like dependencies deprecating things when you don't have a Cargo.lock checked in) should not make my CI red
- Personally, I've never had a dependency make something deprecated so I've not worried about that state before. This is more in theoretically possible land
- I should be able to maintain my MSRV ie deprecation warnings for Rust can only be emitted by the MSRV or else I can't get a "non-red" to approve my submissions.
As long as those conditions are met, I'm fine.
There was a problem hiding this comment.
Okay, so, I think this satisfies all of those. Specifically, the default stages.yml will never make your CI red from warnings, it will only ever make it yellow. You can choose to directly depend on cargo-check.yml, and then you can choose whether you want warnings to cause red CI (err_on_warn: true + allow_fail: false), yellow CI (err_on_warn: true + allow_fail: true), or green CI (err_on_warn: false + allow_fail: false; the default).
Thus, anything that'd make your CI red would be by choice in setting up your pipeline.
Now, what this PR does not give you the ability to do is to run CI and have it fail on warnings that are not due to dependencies. I think that's specifically something you would like, but I also think that's something that's better handled in a new PR where we can explicitly figure out which warnings that would require allowing.
So, with all that said, my inclination would be to merge and then open an issue tracking "red CI from non-dependency warnings".
|
FWIW, I like the idea of (optionally) erroring stable CI for warnings. I think denying warnings at compile time is too annoying for my development experience, but I want code to be warning-free before it hits master. |
|
@djc there are two tricky parts about this:
These taken together mean that failing CI on warnings, even on |
Codecov Report
@@ Coverage Diff @@
## master #33 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 1 1
Lines 8 8
=====================================
Hits 8 8Continue to review full report at Codecov.
|
This PR does two things.
First, it adds a new smoke test to the mix:
cargo check -- -D warnings, which will error on any warning fromrustc.Second, it makes it easy to opt out of all canaries (basically anything that is
allow_fail) by settingThis will currently disable:
-D warningstestThis parameter is intended for users that worry about long-running or highly parallel CI, yellow CI ("partially successful").