Check failing with "Resource not accessible by integration"

[yoshuawuyts]

In https://github.com/async-rs/async-std/pull/271/checks?check_run_id=247986116 we're getting an error about clippy failing:

I'm unsure what's causing this. I'd suspect a bad token of sorts, but I don't see how that could be possible. I saw the note in the readme, but this seems distinct also? Do you maybe know what might be going on? Thanks!

[bjorn3]

I saw the note in the readme, but this seems distinct also?

I think this case is covered by that note, as the pr does have a fork as source branch (sunjay:fromstream-impls)

[yoshuawuyts]

@bjorn3 ahh okay, I probably misinterpreted it then. It'd be nice if this action could detect whether the check is from another repo, and fall back to regular reporting instead (rather than failing)

[svartalf]

Yeah, @bjorn3 is right about the forked repo. We were investigating this problem earlier today with @Luni-4 and I had created a post about this problem at the github forums, but that is pretty much it.

I like an idea about falling back to the old plain stdout reporting, though (thanks, @yoshuawuyts!), that seems like a only reasonable option till Github will handle this problem somehow.

[Luni-4]

I like an idea about falling back to the old plain stdout reporting, though (thanks, @yoshuawuyts!), that seems like a only reasonable option till Github will handle this problem somehow.

Yep, I agree with this option too. In this way the Github Token isn't used at all for now.

[svartalf]

Alright, I added the stdout fallback as was suggested and published it already.
@yoshuawuyts can you restart that check to be sure that everything is okay there?

[yoshuawuyts]

@svartalf thanks so much!

Though https://github.com/async-rs/async-std/pull/271/checks?check_run_id=249044165 seems like it falls back nicely now, but now also always exits with status code 0 even if clippy checks fail.

[svartalf]

@yoshuawuyts right now it is an intended behavior, as the Action returns a non-zero exit code in case if any ICE or errors were found, but warning, note and help message types are considered as a "successful" result.
I was thinking about some kind of strict: true flag, but not sure yet about how should it look in a whole. Would love to hear your ideas about it!

[yoshuawuyts]

@svartalf oh yeah, strict: true seems like a reasonable solution! -- Generally for cargo check we also like to pass RUSTFLAGS="-D warnings" to reject all warnings; if that's not part of the builder yet.

Riffing on the name slightly; if we were to use "rusty" names for this then -D warnings would probably best be translated to reject_warnings: true.

[svartalf]

@yoshuawuyts reject_warnings: true sounds nice!

Here is a contrary thought, since we are already discussing it here: should not the project itself tune the clippy errors via #![deny(clippy::*)] attributes instead? Otherwise local cargo clippy and CI one are going be inconsistent (or callers must be forced to remember proper command to execute each time?)

[svartalf]

Also, it is already possible to add -D warnings flag via the args input like this:

- uses: actions-rs/clippy-check@v1
  with:
    args:  --all-targets --all-features -- -D warnings

It kinda looks like a hack, since the args input is just passed to cargo clippy invocation as is, but it is a working hack :)

[yoshuawuyts]

should not the project itself tune the clippy errors via #![deny(clippy::*)] attributes instead?

So in async-std we used to have this, but switched the flag to only run in CI because we wanted a slightly better cadence during development. E.g. when you're developing features you want to get things to compile, and not bother too much with the warnings much while you're working on it.

Then once you're ready to file a PR you want to have the quality enforced, and ensure that no warnings are merged into the main codebase. I think the inconsistency here is a feature, and overall leads to a much better developer experience!

It's probably possible to set this up through features and configs, but this feels like the kind of thing that having a simple toggle for would allow it to be documented in a single place, easy to use, and in turn make it more accessible to people!

[svartalf]

This looks like a tracking issue for annotations: actions/toolkit#186