Allow instance-wide disabling of forking #2445

algernon · 2024-02-23T08:18:25+01:00

algernon commented

2024-02-23 08:18:25 +01:00

For small, personal self-hosted instances with no user signups, the fork button is just a noise. This patch allows disabling them like stars can be disabled too.

Disabling forks does not only remove the buttons from the web UI, it also disables the routes that could be used to create forks.

Fixes #2441.

For small, personal self-hosted instances with no user signups, the fork button is just a noise. This patch allows disabling them like stars can be disabled too. Disabling forks does not only remove the buttons from the web UI, it also disables the routes that could be used to create forks. Fixes #2441.

👍 4

algernon added the

enhancement/feature

label

2024-02-23 08:18:25 +01:00

algernon force-pushed f/disable-forks from bbb9efdbed

testing / backend-checks (pull_request) Failing after 2m9s

Details

testing / test-unit (pull_request) Has been skipped

Details

testing / test-mysql (pull_request) Has been skipped

Details

testing / test-pgsql (pull_request) Has been skipped

Details

testing / test-sqlite (pull_request) Has been skipped

Details

to 4d8b45879c

testing / backend-checks (pull_request) Successful in 3m46s

Details

testing / test-unit (pull_request) Successful in 6m12s

Details

testing / test-mysql (pull_request) Successful in 14m47s

Details

testing / test-sqlite (pull_request) Successful in 15m14s

Details

testing / test-pgsql (pull_request) Successful in 17m59s

Details

2024-02-23 09:17:31 +01:00

Compare

Gusted reviewed

2024-02-23 12:49:45 +01:00

Gusted left a comment

You're likely already aware, but the checks for the backend are missing.

modules/setting/repository.go Outdated

					
				@ -50,6 +50,7 @@ var (

						PrefixArchiveFiles                      bool

						DisableMigrations                       bool

						DisableStars                            bool `ini:"DISABLE_STARS"`

						DisableForks                            bool `ini:"DISABLE_FORKS"`

I don't see why the ini key should be set for DisableStars or DisableForks. They will be automatically converted to the correct value.

I don't see why the `ini` key should be set for `DisableStars` or `DisableForks`. They will be automatically converted to the correct value.

👍 1

I'll remove the ini key for DisableForks, but I'd rather remove the one for DisableStars in a separate pull request, because doing that in this one would be touching code that's unrelated to what the PR does.

I think there are a couple of other changes that can be made around the DisableStars parts of the code, so I'll submit a separate PR to address those, including removing the ini key here.

I'll remove the `ini` key for `DisableForks`, but I'd rather remove the one for `DisableStars` in a separate pull request, because doing that in this one would be touching code that's unrelated to what the PR does. I think there are a couple of other changes that can be made around the `DisableStars` parts of the code, so I'll submit a separate PR to address those, including removing the `ini` key here.

algernon marked this conversation as resolved

VadZ commented

2024-02-23 19:22:08 +01:00

First-time contributor

I'd love to have some feedback meanwhile about whether a feature like this is worth doing in the first place.

Definitely! We modify the templates in our own instance just to prevent this from being possible.

BTW, our main rationale for doing it is that we forking our main repository, which takes several GBs, would eat all of the available disk space pretty quickly if everyone did it.

> I'd love to have some feedback meanwhile about whether a feature like this is worth doing in the first place. Definitely! We modify the templates in our own instance just to prevent this from being possible. BTW, our main rationale for doing it is that we forking our main repository, which takes several GBs, would eat all of the available disk space pretty quickly if everyone did it.

inference commented

2024-02-23 23:08:05 +01:00

First-time contributor

I'd love to have some feedback meanwhile about whether a feature like this is worth doing in the first place.

Yes. I would like this on my server which has single-user repositories. If we can disable stars, there is no reason to not be able to disable forks, too. Cleaning up the UI of a never-used and can't-be-used feature is essential to comfortable workflow for many people, including myself.

> I'd love to have some feedback meanwhile about whether a feature like this is worth doing in the first place. Yes. I would like this on my server which has single-user repositories. If we can disable stars, there is no reason to not be able to disable forks, too. Cleaning up the UI of a never-used and can't-be-used feature is essential to comfortable workflow for many people, including myself.

algernon force-pushed f/disable-forks from 4d8b45879c

testing / backend-checks (pull_request) Successful in 3m46s

Details

testing / test-unit (pull_request) Successful in 6m12s

Details

testing / test-mysql (pull_request) Successful in 14m47s

Details

testing / test-sqlite (pull_request) Successful in 15m14s

Details

testing / test-pgsql (pull_request) Successful in 17m59s

Details

to 0ea021c8c9

testing / backend-checks (pull_request) Successful in 5m4s

Details

testing / test-unit (pull_request) Successful in 8m11s

Details

testing / test-mysql (pull_request) Successful in 16m35s

Details

testing / test-sqlite (pull_request) Successful in 15m32s

Details

testing / test-pgsql (pull_request) Successful in 18m1s

Details

2024-02-25 12:00:28 +01:00

Compare

algernon changed title from ~~WIP: Allow instance-wide disabling of forking~~ to Allow instance-wide disabling of forking

2024-02-25 12:00:34 +01:00

algernon requested reviews from caesar, crystal, Gusted, earl-warren

2024-02-25 12:00:34 +01:00

algernon commented

2024-02-25 12:03:11 +01:00

Updated the PR:

Increased the scope of DISABLE_FORKS = true: it will now disable the routes that can create forks, rather than just hiding buttons from the UI.
Removed the ini key from the setting struct.
Wrote tests that exercise the disabling functionality:
- Check that the fork button isn't present
- Check that fork-related routes (including listing forks) return 404.

This is ready for review now.

Updated the PR: - Increased the scope of `DISABLE_FORKS = true`: it will now disable the routes that can create forks, rather than just hiding buttons from the UI. - Removed the `ini` key from the setting struct. - Wrote tests that exercise the disabling functionality: - Check that the fork button isn't present - Check that fork-related routes (including listing forks) return 404. This is ready for review now.

earl-warren approved these changes

2024-02-25 16:12:09 +01:00

earl-warren merged commit 649ca2b230 into forgejo

2024-02-25 16:13:17 +01:00

earl-warren referenced this pull request from a commit

2024-02-25 16:13:20 +01:00

Merge pull request 'Allow instance-wide disabling of forking' (#2445) from algernon/forgejo:f/disable-forks into forgejo

algernon deleted branch f/disable-forks

2024-02-26 09:26:36 +01:00

0ko referenced this pull request

2024-02-27 05:45:52 +01:00

[BUG] Fork count is broken #2494