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.

Sign up for GitHub

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

Jump to bottom

Disable unnecessary GitHooks elements #18485

Merged
merged 5 commits into from
Apr 26, 2022
Merged

Disable unnecessary GitHooks elements #18485

merged 5 commits into from
Apr 26, 2022

Conversation

pboguslawski
Copy link
Contributor

This mod fixes disabling unnecessary GitHooks elements.

Related: #13129
Author-Change-Id: IB#1115251

@pboguslawski
Disable unnecessary GitHooks elements
6801919 
This mod fixes disabling unnecessary GitHooks elements.

Related: go-gitea#13129
Author-Change-Id: IB#1115251
@pboguslawski pboguslawski mentioned this pull request Jan 31, 2022
Closed
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 31, 2022
@pboguslawski
Div hiding fixed
6437b04 
Fixes: 6801919
Author-Change-Id: IB#1115251
@pboguslawski
Div hiding fix reverted
8a0eb58 
Placing hidden as CSS class only greys out this option; we want
to hide it. According to

https://stackoverflow.com/questions/1992114/how-do-you-create-a-hidden-div-that-doesnt-create-a-line-break-or-horizontal-sp/27548863#27548863

using hidden as attribute is ok and works ok in gitea.

Fixes: 6437b04
Author-Change-Id: IB#1115251
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 1, 2022
@CirnoT
Copy link
Contributor

CirnoT commented Feb 1, 2022 •
edited
Loading

I do not like these changes. All they do is hide a button in UI while adding additional complexity for no significant gain. In addition they introduce hidden behavior to

[cron.resync_all_hooks]
ENABLED = true

Where above will be ignored if DISABLE_GIT_HOOKS is configured. This isn't good because it makes it that much harder for someone to understand the configuration changes.

I also see no good reason to either hide the button or disable this task - it takes no user input and I fear that this will introduce additional issues where potential update to hooks in newer version will not be propagated to users that applied your new configuration (the task is disabled during any potential migration too or during runtime on startup). While you will no doubt argument that this is something that should be done manually I see no reason why it must be done so and I also find your reasoning flawed and outdated given current practices of spinning up ephemeral instances in swarms.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 2, 2022
lunny
lunny reviewed Feb 2, 2022
View reviewed changes
services/cron/tasks_extended.go Outdated
@@ -160,7 +160,9 @@ func initExtendedTasks() {
registerGarbageCollectRepositories()
registerRewriteAllPublicKeys()
registerRewriteAllPrincipalKeys()
registerRepositoryUpdateHook()
if !setting.DisableGitHooks {
Copy link
Member

Choose a reason for hiding this comment

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

No, I don't think this is right. DisableGitHooks should be DisableExtraGitHooks. We need to update Gitea internal git hooks.

lunny
lunny previously requested changes Feb 2, 2022
View reviewed changes
Copy link
Member

@lunny lunny left a comment

Choose a reason for hiding this comment

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

Gitea supports internal git hooks and extra git hooks editable from UI. The first is a MUST, the second could be disabled. I think the name DisableGitHooks make something confusing.

@wxiaoguang
Copy link
Contributor

Gitea supports internal git hooks and extra git hooks editable from UI. The first is a MUST, the second could be disabled. I think the name DisableGitHooks make something confusing.

Agree to make the system more clear and more consistent.

@pboguslawski
Copy link
Contributor Author

All they do is hide a button in UI while adding additional complexity for no significant gain.

Cleaner app UI is significant gain for this app.

Gitea supports internal git hooks and extra git hooks editable from UI.

Fixed 97a5d84.

@CirnoT
Copy link
Contributor

CirnoT commented Feb 2, 2022

Cleaner app UI is significant gain for this app.

I don't particularly agree on this point. You can use custom themes if you really want to hide some UI element and as it stands out, after 97a5d84 this PR does not even prevent user from issuing request, it just hides a button for absolutely no gain other than hiding it.

@pboguslawski
Copy link
Contributor Author

after 97a5d84 this PR does not even prevent user from issuing request, it just hides a button for absolutely no gain other than hiding it.

This PR currently hides unnecessary git hooks changing permission checkbox in user configuration.

@wxiaoguang wxiaoguang added the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Feb 2, 2022
@techknowlogick techknowlogick added this to the 1.17.0 milestone Feb 2, 2022
@lunny lunny dismissed their stale review February 3, 2022 06:31

Problem resolved.

@6543
Copy link
Member

6543 commented Apr 25, 2022

is it still blocked ?

@wxiaoguang wxiaoguang removed the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Apr 26, 2022
@wxiaoguang
Copy link
Contributor

@CirnoT

@CirnoT
Copy link
Contributor

CirnoT commented Apr 26, 2022

Looks good at the moment, the PR basically hides Can edit Git hooks checkbox on user edit page in administrative dashboard when Git hooks are disabled. The decision whether it's worth it or not belongs to maintainers at this moment - do note however that there is already a check in place to set disabled attribute on checkbox.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 26, 2022 •
edited
Loading

The comment is pretty good and more clear.

I agree with keeping showing the Can edit Git hooks is more clear to admins (it's a user's attribute on the page, not a feature module). Hiding it is not bad too.

Although my preference is to make UI consistent instead of showing/hiding tiny elements dynamically (and personally I would vote for merging the new comments, reverting the HTML element hiding), either is fine to me.

6543
6543 approved these changes Apr 26, 2022
View reviewed changes
Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

since it's default is disabled - I'm also for hiding

@zeripath zeripath merged commit 0b38084 into go-gitea:main Apr 26, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 28, 2022
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
509d619 
* giteaofficial/main: (21 commits)
  Prevent intermittent race in attribute reader close (go-gitea#19537)
  Make repository file list useable on mobile (go-gitea#19515)
  Update image URL for Discord webhook (go-gitea#19536)
  [skip ci] Updated translations via Crowdin
  Fix 64-bit atomic operations on 32-bit machines (go-gitea#19531)
  Fix `upgrade.sh` script error with `su -c` (go-gitea#19483)
  When view _Siderbar or _Footer, just display once (go-gitea#19501)
  Fix migrate release from github (go-gitea#19510)
  Prevent dangling archiver goroutine (go-gitea#19516)
  Don't let repo clone URL overflow (go-gitea#19517)
  Add commit status popup to issuelist (go-gitea#19375)
  Disable unnecessary GitHooks elements (go-gitea#18485)
  Disable unnecessary GitHooks elements
  Improve dashboard's repo list performance (go-gitea#18963)
  By default force vertical tabs on mobile (go-gitea#19486)
  Refactor readme file renderer (go-gitea#19502)
  Allow package dump skipping (go-gitea#19506)
  Unset git author/committer variables when running integration tests (go-gitea#19512)
  Allow commit status popup on /pulls page (go-gitea#19507)
  Use router param for filepath in GetRawFile (go-gitea#19499)
  ...
@pboguslawski pboguslawski deleted the main-IB#1115251 branch May 26, 2022 15:34
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
@pboguslawski @AbdulrhmnGhanem
Disable unnecessary GitHooks elements (go-gitea#18485)
22a5ce8 
* Disable unnecessary GitHooks elements

This mod fixes disabling unnecessary GitHooks elements.

Related: go-gitea#13129
Author-Change-Id: IB#1115251
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@silverwind silverwind silverwind approved these changes

@6543 6543 6543 approved these changes

@lunny lunny lunny left review comments

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore.
Projects
None yet
Milestone
1.17.0
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@pboguslawski @CirnoT @wxiaoguang @6543 @lunny @silverwind @techknowlogick @zeripath @GiteaBot