Skip to content

Make hook status printing configurable with delay#9641

Merged
zeripath merged 9 commits into
go-gitea:masterfrom
zeripath:fix-9610
Jan 12, 2020
Merged

Make hook status printing configurable with delay#9641
zeripath merged 9 commits into
go-gitea:masterfrom
zeripath:fix-9610

Conversation

@zeripath
Copy link
Copy Markdown
Contributor

@zeripath zeripath commented Jan 7, 2020

Add 2 configuration options to control printing of push hook status with a configurable delay.

[git]
...
VERBOSE_PUSH=true
VERBOSE_PUSH_DELAY=5s

Fix #9610

@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #9641 into master will decrease coverage by 0.03%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9641      +/-   ##
==========================================
- Coverage   42.17%   42.14%   -0.04%     
==========================================
  Files         583      583              
  Lines       77160    77220      +60     
==========================================
  Hits        32541    32541              
- Misses      40616    40676      +60     
  Partials     4003     4003
Impacted Files Coverage Δ
cmd/hook.go 0% <0%> (ø) ⬆️
routers/repo/view.go 38.59% <0%> (-0.88%) ⬇️
services/pull/check.go 52.81% <0%> (-0.71%) ⬇️
models/pull.go 46.06% <0%> (-0.49%) ⬇️
models/repo.go 47.97% <0%> (+0.1%) ⬆️
services/pull/merge.go 53.92% <0%> (+0.78%) ⬆️
services/pull/patch.go 64.55% <0%> (+1.89%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b9309e5...db26235. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 7, 2020
Copy link
Copy Markdown
Member

@guillep2k guillep2k left a comment

Choose a reason for hiding this comment

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

I've written a bunch of comments and suggestions, but I'm not sure this is the right approach. I realize you want to be faithful to the previous state, where a dot was printed for each commit that has been checked. But maybe it would be much easier if we'd just print a dot for each second that has passed after the initial delay (with no goroutines or string builders).

Comment thread cmd/hook.go Outdated
Comment thread cmd/hook.go Outdated
Comment thread cmd/hook.go Outdated
Comment thread cmd/hook.go Outdated
Comment thread cmd/hook.go Outdated
Comment thread cmd/hook.go Outdated
@zeripath zeripath changed the title Delay printing hook statuses until after 1 second Make hook status printing configurable with delay Jan 11, 2020
@zeripath zeripath added the type/enhancement An improvement of existing functionality label Jan 11, 2020
@zeripath zeripath added this to the 1.11.0 milestone Jan 11, 2020
@zeripath
Copy link
Copy Markdown
Contributor Author

I've moved to a wrapped writer implementation with a configurable delay.

@zeripath zeripath modified the milestones: 1.11.0, 1.12.0 Jan 11, 2020
@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 Jan 11, 2020
@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 Jan 11, 2020
Copy link
Copy Markdown
Member

@guillep2k guillep2k left a comment

Choose a reason for hiding this comment

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

Nice!

Comment thread cmd/hook.go
Comment thread cmd/hook.go Outdated
Comment thread cmd/hook.go Outdated
Comment thread cmd/hook.go Outdated
Comment thread cmd/hook.go Outdated
@sapk
Copy link
Copy Markdown
Member

sapk commented Jan 12, 2020

I stopped and restarted the build which seems blocked.

Comment thread cmd/hook.go Outdated
@zeripath
Copy link
Copy Markdown
Contributor Author

@sapk yup that was my fault. The documentation and example for New timer is particularly bad. I even knew about this problem but forgot about it.

@zeripath
Copy link
Copy Markdown
Contributor Author

Make lg-tm work

@zeripath zeripath merged commit 65baacf into go-gitea:master Jan 12, 2020
zeripath added a commit to zeripath/gitea that referenced this pull request Jan 12, 2020
* Delay printing hook statuses until after 1 second

* Move to a 5s delay, wrapped writer structure and add config

* Update cmd/hook.go

* Apply suggestions from code review

* Update cmd/hook.go

Co-authored-by: Antoine GIRARD <sapk@users.noreply.github.com>
zeripath added a commit that referenced this pull request Jan 12, 2020
* Delay printing hook statuses until after 1 second

* Move to a 5s delay, wrapped writer structure and add config

* Update cmd/hook.go

* Apply suggestions from code review

* Update cmd/hook.go

Co-authored-by: Antoine GIRARD <sapk@users.noreply.github.com>

Co-authored-by: Antoine GIRARD <sapk@users.noreply.github.com>
@zeripath zeripath deleted the fix-9610 branch January 13, 2020 12:52
@zeripath zeripath added the backport/done All backports for this PR have been created label Jan 13, 2020
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Disable remote messages on push

7 participants