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

Default behavior for forks of this workflow is frustrating #1502

Open
jsoref opened this issue Feb 4, 2025 · 6 comments
Open

Default behavior for forks of this workflow is frustrating #1502

jsoref opened this issue Feb 4, 2025 · 6 comments

Comments

@jsoref
Copy link
Contributor

jsoref commented Feb 4, 2025

I run repositories where my default branch often doesn't match the default branch of the upstream repository. This is a totally reasonable thing for me to do.

When I refresh my repository with the latest master, I get this unhappy view:
https://github.com/check-spelling-sandbox/cert-manager/actions/runs/13138950431

Image

Problems:

  1. There should be a step summary that says whatever is interesting

  2. If there isn't a step summary, and the action wants to trigger a ❌ , it should surface an annotation explaining why it's unhappy, which I presume is:

    refs/heads/master not supported with push event.
    Only the default branch spell-check is supported.
    2025/02/04 15:29:20 creating scorecard entrypoint: validating options: only default branch is supported
    
  3. Assuming the action is only willing to run when run from the default branch, then the default suggested workflow should include a guard so that the workflow doesn't run when the default branch doesn't match the current branch. The guard is simple: cert-manager/cert-manager@d3fb179

  4. As users rarely update actions in existing workflows, the action should nudge people to add the guard by including a step summary explaining how to add the guard.

I'm willing to make PRs for these items, but can't guarantee that I'll make them promptly as this is very low on my yak list.

@spencerschrock
Copy link
Member

  1. There should be a step summary that says whatever is interesting

Is the distinction between step summary and annotation just that you can better tell which step is generating the message?

For example your screenshot has two deprecation warnings that are coming from outdated version of actions/checkout.

If there isn't a step summary, and the action wants to trigger a ❌ , it should surface an annotation explaining why it's unhappy, which I presume is:

Agreed. #1459 was a step in the right direction (and would have helped you here, pending next release #1473 ), but there are several other places in that file that could be interesting:

  • Empty token / auth instructions
  • Unsupported event types
  • no result file specified

Plus any number of possible Scorecard errors during the analysis. And the upload steps / signing

Assuming the action is only willing to run when run from the default branch, then the default suggested workflow should include a guard so that the workflow doesn't run when the default branch doesn't match the current branch

We do gate on the $default-branch variable in the starter workflow, but I see how that falls short for forks. Your suggestion makes sense. We haven't gotten this feedback before, presumably because GitHub disables actions by default in forks. I assume you manually enabled it?

As users rarely update actions in existing workflows, the action should nudge people to add the guard by including a step summary explaining how to add the guard.

Depending on what you mean by "update", users wouldn't necessary see the nudge if they don't update to the version with the nudge. (e.g. the outdated actions/checkout and ossf/scorecard-action)

@jsoref
Copy link
Contributor Author

jsoref commented Feb 4, 2025

Step summaries vs Annotations

Is the distinction between step summary and annotation just that you can better tell which step is generating the message?

Step summaries

Step summaries are much prettier and can have paragraphs of information. Here's a step summary from that repository: https://github.com/check-spelling-sandbox/cert-manager/actions/runs/13144281591#summary-36678608798.

Annotations

You can control the title and the message, but if the message is too long, it'll be partially clipped by a show more option...

Check Spelling
Process completed with exit code 1.

Actions in forks

We do gate on the $default-branch variable in the starter workflow, but I see how that falls short for forks. Your suggestion makes sense. We haven't gotten this feedback before, presumably because GitHub disables actions by default in forks. I assume you manually enabled it?

I'm pretty sure the act of pushing a changed workflow to a fork enables actions in forks. But it hardly matters to me, I'm actively using my forks to run workflows (as you can see w/ the step summary above), so, yes, I have actions enabled in my forks.

Nudges

Depending on what you mean by "update", users wouldn't necessary see the nudge if they don't update to the version with the nudge. (e.g. the outdated actions/checkout and ossf/scorecard-action)

At some point, upstream will be nudged (probably by dependabot, or a competitor) to upgrade ossf/scorecard-action, that will give them a newer version of ossf/scorecard-action, but it won't fix the workflow. When the workflow then is pulled into downstreams that run workflows, your code can nudge them to tell the upstream to add the guard.

@jsoref
Copy link
Contributor Author

jsoref commented Feb 4, 2025

Oh, I should note that most github users have better things to do than to send feedback to upstreams or component upstreams, as action authors we may feel like we're getting overwhelmed by feedback, but most feedback is never composed let alone sent. I think I'm quite rare in that I'll spend the time to send feedback to the various components that are misbehaving...

Users are more likely to ignore the problem, or disable the workflows than try to solve it, especially since the individual workflow authors tend to respond with "so turn off our workflows, we don't support them in forks anyway, stop wasting our time" (and yes, I get that a lot).

@spencerschrock
Copy link
Member

3. The guard is simple: cert-manager/cert-manager@d3fb179

I don't believe the guard works as intended:

github.repository.default_branch resolves to an empty string for me. So for you the workflow isn't running because "" != "spell-check"

github.repository | string | The owner and repository name. For example, octocat/Hello-World

Outside of an API call to get the default branch, I assume the default branch name would need to be hardcoded into the expression.

# or "master" depending on the repo
if: github.ref_name == "main"

Note: this would still work for the starter action template because of the $default-branch variable.

@spencerschrock
Copy link
Member

I see github.event.repository.default_branch is a thing, and defined for push and pull_request events, so I assume that's what you meant?

@jsoref
Copy link
Contributor Author

jsoref commented Feb 5, 2025

Oops.

I was looking at https://github.com/jsoref/debug-github-events/actions/runs/12938525337 and missed a layer in the hierarchy.

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

No branches or pull requests

2 participants