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

Refactor process package and introduce ProcessManager{} with tests #75

Merged
merged 4 commits into from
Jan 17, 2017
Merged

Refactor process package and introduce ProcessManager{} with tests #75

merged 4 commits into from
Jan 17, 2017

Conversation

metalmatze
Copy link
Contributor

This is still WIP and other files need to be updated.
Some funcs that use this need get a ProcessManager instance passed.

@strk strk added pr/wip This PR is not ready for review type/enhancement An improvement of existing functionality labels Nov 5, 2016
tboerger
tboerger reviewed Nov 5, 2016
View reviewed changes
modules/process/manager.go
if timeout == -1 {
timeout = DEFAULT
timeout = DefaultTimeout
Copy link
Member

Choose a reason for hiding this comment

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

Just set the default within the exec function?

@tboerger tboerger added this to the 1.0.0 milestone Nov 5, 2016
@metalmatze metalmatze changed the title WIP: Refactor process package and introduce ProcessManager{} with tests Refactor process package and introduce ProcessManager{} with tests Nov 6, 2016
@metalmatze
Copy link
Contributor Author

This is ready to merge.

lunny
lunny reviewed Nov 6, 2016
View reviewed changes
modules/process/manager.go

// Add a process to the ProcessManager and returns its PID.
func (pm *Manager) Add(description string, cmd *exec.Cmd) int64 {
pid := pm.counter + 1
Copy link
Member

Choose a reason for hiding this comment

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

Should this line put after the lock line?

Copy link
Member

Choose a reason for hiding this comment

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

^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not necessarily. But probably better.

@strk strk modified the milestones: 1.1.0, 1.0.0 Nov 6, 2016
@tboerger
Copy link
Member

tboerger commented Nov 7, 2016

@metalmatze travis is failing.

@strk strk removed the pr/wip This PR is not ready for review label Nov 7, 2016
@metalmatze
Copy link
Contributor Author

Travis fails because there's no openssh-client on travis. I think I'll leave this for now, until we're drone only. Then this will pass.

@tboerger tboerger added the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Nov 11, 2016
@lunny lunny mentioned this pull request Nov 15, 2016
Merged
@strk
Copy link
Member

strk commented Nov 15, 2016

@metalmatze did you try adding openssh-client to the apt-get install line in before_install of .travis.yml ?

@lunny
Copy link
Member

lunny commented Nov 15, 2016

This PR is waiting until drone is ready. @tboerger

@strk
Copy link
Member

strk commented Nov 15, 2016

Drone PR is #96

@strk strk mentioned this pull request Nov 17, 2016
Merged
@tboerger tboerger modified the milestones: 1.0.0, 1.1.0 Nov 24, 2016
@tboerger tboerger removed the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Nov 25, 2016
@lunny
Copy link
Member

lunny commented Nov 25, 2016

what's the block?

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 25, 2016
@tboerger
Copy link
Member

I have dropped the blocked panel

@lunny
Copy link
Member

lunny commented Nov 25, 2016

So this only needed to rebase

@tboerger
Copy link
Member

Yes, and @metalmatze said that he will finish it this weekend

@tboerger
Copy link
Member

tboerger commented Dec 3, 2016

Do we want to move it to 1.1 @metalmatze?

@lunny
Copy link
Member

lunny commented Dec 5, 2016

I just move this to 1.1 and if @metalmatze back and 1.0 is not released yet, please move it back.

@lunny lunny modified the milestones: 1.1.0, 1.0.0 Dec 5, 2016
@lunny
Copy link
Member

lunny commented Dec 25, 2016

Please rebase and we can review continue.

@tboerger
Copy link
Member

@metalmatze can you finish that and resolve the conflicts?

@tboerger tboerger added the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label Dec 29, 2016
@metalmatze
Copy link
Contributor Author

Yes I want to, but currently at 33C3 there's to less time. I'll try to rebase and finish on the weekend.

@metalmatze
Copy link
Contributor Author

Ok. Instead of rebasing I've now copied over my changes in modules/process and simply refactored all its use cases.

@lunny
Copy link
Member

lunny commented Jan 16, 2017

Is this ready for review again?

@lunny lunny removed the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label Jan 16, 2017
@metalmatze
Copy link
Contributor Author

I guess. I fixed all the errors and rebased to the current master.

lunny
lunny requested changes Jan 16, 2017
View reviewed changes
modules/process/manager.go

// Add a process to the ProcessManager and returns its PID.
func (pm *Manager) Add(description string, cmd *exec.Cmd) int64 {
pid := pm.counter + 1
Copy link
Member

Choose a reason for hiding this comment

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

^

@lunny
Copy link
Member

lunny commented Jan 16, 2017

otherwise LGTM

@tboerger tboerger 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 16, 2017
@tboerger
Copy link
Member

LGTM

@tboerger tboerger 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 16, 2017
@lunny lunny merged commit d100615 into go-gitea:master Jan 17, 2017
@metalmatze metalmatze deleted the feature/process branch January 17, 2017 08:53
lunny pushed a commit to lunny/gitea that referenced this pull request Feb 7, 2019
@melak @appleboy
Trim commit message when generating Summary (go-gitea#75)
ef3ac5f 
Commit messages with superfluous empty lines on the top generate
empty summaries. Trim the commit message so the summary contains
something meaningful (as far as the commit message permits).
@cruwe cruwe mentioned this pull request Nov 12, 2019
Closed
7 tasks
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tboerger tboerger tboerger left review comments

@lunny lunny lunny approved these changes

Assignees
No one assigned
Labels
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
Milestone
1.1.0
Development

Successfully merging this pull request may close these issues.
4 participants
@metalmatze @tboerger @strk @lunny