Skip to content

Tweak the publication condition to prevent publishing from forked repos #810

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

danicheg
Copy link
Member

@danicheg danicheg commented Apr 6, 2025

One might argue that users can use githubWorkflowPublishCond to prevent publishing from forked repos. Sure, but I’m confident this condition should be the default. Adding it manually to the build of dozens of projects isn’t exactly ideal, either.
This is basically the same as what we did in #720, but adding the same SBT setting again just feels a bit extra to me 🤷🏻

@danicheg
Copy link
Member Author

danicheg commented Apr 6, 2025

I was just thinking again about adding a dedicated SBT setting for the fork condition. Maybe introducing githubWorkflowForkCondition with a default value of github.event.repository.fork == false isn't such a bad idea — especially if we deprecate the tlCiForkCondition and substitute its default with the newly added githubWorkflowForkCondition. WDYT?

@bpholt
Copy link
Member

bpholt commented Apr 14, 2025

I think it is a good idea not to publish from forks by default, but I can envision situations where users legitimately would want to do so, so I don't think it should be disabled without a way to re-enable it. A dedicated sbt setting seems reasonable to me.

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, this is a really good change. Sorry it took me so long to review. I have just a couple nitpicks.

@danicheg danicheg requested a review from armanbilge May 20, 2025 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants