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 special error message case for dependabot #435

Merged
merged 3 commits into from
Mar 31, 2021

Conversation

robertbrignull
Copy link
Contributor

This error currently comes through as "Resource not accessible by integration" but it would be helpful if we said something better. I had a hard time writing this error message as I wasn't sure what to say. If other people have other ideas please do let me know.

Here's an example of what it looks like in practice: https://github.com/dsp-testing/robertbrignull-dependabot-test/pull/7/checks?check_run_id=2227856119

I also considered putting some example configuration like branches-ignore: ['dependabot/**'] in the message but then decided against it. I think that would be a bit hard to read in a single-line message like this and I worry about giving explicit practical advice as it's never going to be right for every workflow.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.

@cannist
Copy link
Contributor

cannist commented Mar 30, 2021

I fear the error message might be slightly confusing if the user does not have enough context already. But I cannot come up with something better in the short space we have for an error message. Although perhaps you could add "Uploading code scanning results requires write access." as second sentence?

I wonder if we should have something like a FAQ entry that explains the situation and exemplifies a solution in more detail that we could link to in the error message?
Do we have a FAQ page or something similar for code scanning or the CodeQL action? I cannot seem to find anything like that.

src/actions-util.ts Outdated Show resolved Hide resolved
@adityasharad
Copy link
Contributor

https://docs.github.com/en/code-security/secure-coding/troubleshooting-the-codeql-workflow is the closest to an FAQ page. We can put out a docs update there too.

@rneatherway
Copy link
Contributor

Something that concerns me a bit about this error message is that it is most problematic when the branch gets squash-merged into the default branch, and the wording doesn't quite account for that.

@robertbrignull
Copy link
Contributor Author

I like the idea of putting a more detailed explanation in the docs and then linking to it. I agree https://docs.github.com/en/code-security/secure-coding/troubleshooting-the-codeql-workflow looks like the best place for it.

However in the absence of a docs page I've changed the PR to link to https://docs.github.com/en/code-security/secure-coding/configuring-code-scanning#scanning-on-push (feel free to object if you think this isn't the right choice) and included the "Uploading Code Scanning results requires write access." line that @cannist suggested.

We could merge this now and add a docs page later, or we could hold off on this PR if we'd rather not have this temporarily.

@robertbrignull
Copy link
Contributor Author

Something that concerns me a bit about this error message is that it is most problematic when the branch gets squash-merged into the default branch, and the wording doesn't quite account for that.

I don't know how to explain this aspect in a message like this. This is further evidence that we should opt for a docs troubleshooting page and link there I think.

@aeisenberg
Copy link
Contributor

I think this is good to go in (almost) as is and then we can add a FAQ entry for troubleshooting. When that's available, we can update the link here to point to the FAQ.

Aditya had two small grammar changes that should probably be fixed and then 🚢 .

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.

5 participants