Skip to content

Conversation

@appleboy
Copy link
Member

@appleboy appleboy commented Jan 8, 2017

We can restart the app automatically by the following command.

kill -USR2 $(cat custom/run/app.pid)

@lunny lunny added this to the 1.1.0 milestone Jan 8, 2017
@lunny lunny added type/enhancement An improvement of existing functionality type/feature Completely new functionality. Can only be merged if feature freeze is not active. labels Jan 8, 2017
@lunny
Copy link
Member

lunny commented Jan 8, 2017

Why we need a pid file?

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 8, 2017
@appleboy
Copy link
Member Author

appleboy commented Jan 8, 2017

You can also use that information (just do a cat filename.pid) to kill the process yourself, using echo filename.pid | xargs kill. For develop purpose, you just build the binary and execute kill -USR2 $(cat custom/run/app.pid) command to restart app.

@tboerger
Copy link
Member

tboerger commented Jan 8, 2017

I never needed that before but 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 8, 2017
@thibaultmeyer
Copy link
Contributor

thibaultmeyer commented Jan 8, 2017

What happening if PID file already exist (ie: the app crash) ? app can be restarted without manually delete PID file ?

To avoid any issue or conflict with System D, supervisor, or other kind of tools. I think PID file could be created only if -p flag or explicit configuration on app.ini are specified

@appleboy
Copy link
Member Author

appleboy commented Jan 8, 2017

@0xBAADF00D You don't worry about the PID file exists or not. App can be restarted successfully if you remove the pid file. Just try the following command.

gitea web
rm -rf custom/run/app.pid
kill -USR2 gitea_pid
cat custom/run/app.pid

@Bwko
Copy link
Member

Bwko commented Jan 8, 2017

@appleboy What happens when a startup script also sets a PID?

@appleboy
Copy link
Member Author

appleboy commented Jan 8, 2017

@Bwko What kind of start script do you use?

@ekozan
Copy link

ekozan commented Jan 8, 2017

what about windows ? it's work ?

@appleboy
Copy link
Member Author

appleboy commented Jan 9, 2017

@ekozan Yes. Also working on Windows platform.

@appleboy
Copy link
Member Author

appleboy commented Jan 9, 2017

@Bwko

screen shot 2017-01-09 at 4 53 55 pm

Also working on start-stop-daemon command

Copy link
Contributor

Choose a reason for hiding this comment

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

Please check the error returned by os.MkdirAll().

Copy link
Member Author

Choose a reason for hiding this comment

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

@metalmatze Done! Please review again.

@Bwko
Copy link
Member

Bwko commented Jan 9, 2017

@appleboy Thanks 👍 When you add metalmatze suggestions I'll LGT-M 😄

@appleboy
Copy link
Member Author

appleboy commented Jan 9, 2017

@Bwko Done!

Copy link
Member

Choose a reason for hiding this comment

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

Quoting bkcsoft on this one:

Overall, change the format to this instead whenever possible. Keeps errors(variables) bound to their context(block).

if err := os.Create(...); err != nil { [...] }

Copy link
Member Author

Choose a reason for hiding this comment

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

@Bwko We need file variable.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry my bad 😅

Copy link
Member

Choose a reason for hiding this comment

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

Some nitpicking: missing a t here

@Bwko
Copy link
Member

Bwko commented Jan 9, 2017

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 9, 2017
@lunny
Copy link
Member

lunny commented Jan 9, 2017

need @metalmatze approve

@metalmatze metalmatze merged commit 9717091 into go-gitea:master Jan 9, 2017
@appleboy appleboy deleted the pid branch January 9, 2017 14:26
@appleboy
Copy link
Member Author

appleboy commented Jan 9, 2017

@lunny @Bwko @metalmatze Thanks

@tboerger tboerger removed the type/enhancement An improvement of existing functionality label Jan 24, 2017
@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.

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants