Do not run the 'dependency-submission' job in CI for forked repositories

[danicheg]

This tries to fix the following issue in forked repositories:

[error] Unexpected status 404 Not Found with body:
[error] {"message":"The Dependency graph is disabled for this repository. Please enable it before submitting snapshots.","documentation_url":"https://docs.github.com/rest/dependency-graph/dependency-submission#create-a-snapshot-of-dependencies-for-a-repository","status":"404"}

Example: https://github.com/danicheg/cats/actions/runs/10027887278/job/27714122616#step:10:554

[danicheg]

It looks like it works https://github.com/danicheg/sbt-typelevel/actions/runs/10043935348/job/27757804649

[armanbilge]

Thanks!

This is something I've thought about doing before. We have a similar problem not just with dependency submission but also with publishing: forks also attempt to publish even though their credentials are not setup correctly. So we should also consider fixing that in a similar way.

Although this solution works well for CI, the reason I've hesitated to implement it before is because it requires that the developer's local git repository is correctly configured with the upstream remote. Unfortunately, this does not happen automatically: if you fork a repo on GitHub and then git clone it locally, the upstream will not be configured and until you do so you will not be able to properly generate workflows.

Thoughts?

[armanbilge]

Let's move the event_name condition into a constant so we don't have to duplicate it?

[danicheg]

yeah, it makes sense 👍🏻

[danicheg]

it requires that the developer's local git repository is correctly configured with the upstream remote. Unfortunately, this does not happen automatically: if you fork a repo on GitHub and then git clone it locally, the upstream will not be configured and until you do so you will not be able to properly generate workflows.

Oh, I see. Several thoughts:

1. Because we have a CI check, the issue with the wrong Github Repository Owner will be clearly detected. So, a user might manually fix the issue (it's a one-time action) or configure the git repository properly to meet our requirements.
2. We can have a distinct tlGitHubRepoOwner SBT setting. Users will have the ability to explicitly set it. Or it will be filled in via GitHelper#extractGitHubUserRepo.

[armanbilge]

Thanks for thinking about it!

So, a user might manually fix the issue (it's a one-time action) or configure the git repository properly to meet our requirements.

Is this trade-off worth it?

I guess the question I'm wondering is, what is the harm of this failed CI job on forked repositories? We already have failed CI for publishing as well.

Also, if it really bothers users, why don't they take the "one-time action" to enable Dependency Graph for their repository? Wouldn't that solve the problem?

[danicheg]

Also, if it really bothers users, why don't they take the "one-time action" to enable Dependency Graph for their repository? Wouldn't that solve the problem?

There seems to be some confusion when we talk about 'users.' From my perspective, a typical user of sbt-typelevel is a maintainer of an affiliate project. In 99% of cases, these users are the ones who set up CI workflows in their projects. We can reasonably expect them to have decent skills in a wide range of areas, such as setting up a local git repository. On the other hand, there are users of these affiliate projects. These users are more likely to focus on new features, bug fixing, and enhancing documentation in their typical contributions, rather than adjusting the CI workflow. However, these users might encounter confusion in their forks when running CI workflows. To sum up:
This work aims to improve the DX of contributors (by reducing the confusion caused by failing CI in their forks) of affiliate projects, rather than focusing on the maintainers of these projects. Let's be honest with ourselves, nowadays we can count them on one hand.

[armanbilge]

On the other hand, there are users of these affiliate projects. These users are more likely to focus on new features, bug fixing, and enhancing documentation in their typical contributions, rather than adjusting the CI workflow.

Ah yes, I see your point. I agree.

I do have one last idea, that would make workflow generation independent of the git repository configuration, by using a condition like this:

if: github.event.repository.fork == false

I think what we should do is serialize this as a setting e.g. tlCiForkCondition with that as the default value. In the unlikely situation that the primary repository is a fork (e.g. of some unmaintained upstream) then they can reconfigure it appropriately.

WDYT?

[danicheg]

If you argue that this approach feels better, it's okay with me. I'd like to have that issue resolved, no matter the path we take.

[danicheg]

@armanbilge by the way, If I understand things correctly, your suggested solution won't handle the case when the affiliate project is a fork. tlCiForkCondition is set to true, github.event.repository.fork would be computed as true for users' forks — and we'll get the run dependency-submission when it's unsolicited. Should we worry about this case? Perhaps you know better 🤷🏻 I'm not contributing to such projects within Typelevel.

[armanbilge]

If I understand things correctly, your suggested solution won't handle the case when the affiliate project is a fork

By default, no, it would not handle that case. That is why I suggested we implement it as a configurable setting:

I think what we should do is serialize this as a setting e.g. tlCiForkCondition with that as the default value. In the unlikely situation that the primary repository is a fork (e.g. of some unmaintained upstream) then they can reconfigure it appropriately.

Repositories that are maintained as forks are very unusual/rare.

[danicheg]

@armanbilge So, is there anything I need to address here to move on with this?

[armanbilge]

Yes, sorry. I think your concern was legitimate, which is why I suggested we implement this as a setting #720 (comment).

[danicheg]

Feeling dumb, but I altered the initial approach to align with your suggestions. So now we have a configurable setting with a reasonable default value 🤔

[armanbilge]

So sorry, I think I didn't explain very well. What I mean is that we should have String setting tlCiForkCondition which has the default value github.event.repository.fork == false, but if necessary users can customize it to github.repository == '$repo'.

[danicheg]

Returning to this...

What I mean is that we should have String setting tlCiForkCondition which has the default value github.event.repository.fork == false, but if necessary users can customize it to github.repository == '$repo'.

Wouldn't that tlCiForkCondition setting be somewhat of a leaky abstraction? I mean, it wouldn't just specify some behaviour of the underlying GitHub Action but would actually allow the injection of any YAML into the GitHub Action. I don't know if this is already the usual practice in the plugin, though.

[armanbilge]

but would actually allow the injection of any YAML into the GitHub Action

Well, not any YAML, just a conditional expression. And we already accept an arbitrary string anyway.

sbt-typelevel/github-actions/src/main/scala/org/typelevel/sbt/gha/WorkflowStep.scala

Line 22 in e384990

def cond: Option[String]

Any plugin could always manipulate the gitHubWorkflowSteps setting and do arbitrary things to the conds.

[danicheg]

Okay, as I said, any solution would work for me if it resolves the original request.

[danicheg]

@armanbilge hey Arman, what's up? Is there anything else I need to polish to move forward with this?

[armanbilge]

Sorry, just got a bit behind :) this looks good, thanks for adjusting!