Dependabot "Upload SARIF" still not quite right #6
Comments
EliahKagan
added a commit
that referenced
this issue
Sep 5, 2023
Instead of not running on the push trigger for Dependabot branches, which is not fully effective because it still fails on main when a merge is done by Dependabot due to `@dependabot merge` or similar command, this just specifies the required permissions in the workflow file, as is done e.g. in automatically generated CodeQL workflow files and as suggested in #6. I tested this in https://github.com/EliahKagan/hello-world. See EliahKagan/hello-world#14 including EliahKagan/hello-world@d565b84, EliahKagan/hello-world#15, and EliahKagan/hello-world#16.
EliahKagan
added a commit
that referenced
this issue
Sep 5, 2023
Instead of not running on the push trigger for Dependabot branches, which is not fully effective because it still fails on main when a merge is done by Dependabot due to `@dependabot merge` or similar command, this just specifies the required permissions in the workflow file, as is done e.g. in automatically generated CodeQL workflow files and as suggested in #6. I tested this in https://github.com/EliahKagan/hello-world. See EliahKagan/hello-world#14 including EliahKagan/hello-world@d565b84, EliahKagan/hello-world#15, and EliahKagan/hello-world#16.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The changes to the workflow that I added as further commits in #5 prevent unintended failures in an open Dependabot PR, but once the PR is merged, if merged by Dependabot via command like
@dependabot merge, the failure happens on the main branch. So the workflow should be further improved to avoid this.The reason a failure doesn't show on the main branch for where #5 itself was merged is that, although it did originally show that, I created a new branch off main, and that push event worked because it had access to what it needed.
I think it's enough to specify write access explicitly in permissions in the workflow file, but I'll want some confirmation of this before making the change, since it's not really feasible to test all relevant combinations of state that might trigger the problem.
The text was updated successfully, but these errors were encountered: