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.

Sign up for GitHub

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

A better story for canary jobs (yellow CI) #33

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

A better story for canary jobs (yellow CI) #33

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0cc8254
Add warn-free clippy smoketest
jonhoo Jul 16, 2019
2a5d0a8
Make it easy to disable all smoke tests
jonhoo Jul 16, 2019
1375923
Use cargo check with -Dwarnings over clippy
jonhoo Jul 16, 2019
9891ddd
Canaries is better than smoketests
jonhoo Jul 16, 2019
432bb20
Slightly more concise name for -Dwarnings
jonhoo Jul 16, 2019
c0d85e3
cargo check needs setup
jonhoo Jul 16, 2019
0e19cd3
Support allow_fail on cargo check
jonhoo Jul 16, 2019
a413dbb
Remove remains of reverted change
jonhoo Jul 22, 2019
7bcfb49
Merge branch 'master' into smoketest
jonhoo Jul 23, 2019
4e6377a
Avoid a repeated if
jonhoo Jul 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions azure/cargo-check.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@ jobs:
- job: ${{ parameters.name }}
${{ if eq(parameters.rust, 'stable') }}:
${{ if eq(parameters.err_on_warn, 'true') }}:
displayName: "env RUSTFLAGS=-Dwarnings cargo check"
displayName: "cargo check -Dwarnings"
${{ 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"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we allow deprecated? It'll make things more stable.

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 already allow_fail, so I'd be inclined to still give deprecation warnings. They're things you should probably fix, just like other warnings.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. This template does not have an allow_fail
  2. This assumes that the caller will always pass allow_fail with err_on_warn. In my use case, I do not want to be doing that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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".

${{ if ne(parameters.err_on_warn, 'true') }}:
displayName: cargo +${{ parameters.rust }} check
pool:
Expand Down
1 change: 1 addition & 0 deletions azure/style.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ jobs:
- template: cargo-check.yml
parameters:
name: warning_free
setup: ${{ parameters.setup }}
err_on_warn: true
allow_fail: true
- ${{ if eq(parameters.canaries, 'true') }}:
Expand Down