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.

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

Modernized start, stop, restart and new ensure command #4023

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Nico8340
Copy link
Contributor

Modernized start, stop, restart commands which now also support directories, see the examples below, and a new ensure command, which restarts the already running resources specified as parameters, and starts the stopped ones.
Closes #2549.

start play
start admin ip2c playercolors
start [web]

stop play
stop admin ip2c playercolors
stop [web]

ensure play
ensure admin ip2c playercolors
ensure [web]

restart play
restart admin ip2c playercolors
restart [web]

@Lpsd
Copy link
Member

Lpsd commented Feb 14, 2025

We should probably add an argument ensure to restartResource (defaulting to false)?

The reason I don't suggest adding a separate API function for this (e.g: ensureResource), is because we'd need to repeat the arguments and associated logic for those anyway.

Edit: since you'd need to upgrade restartResource to the new arg parser anyway, the code would actually end up being reasonably sized; having an additional function such as ensureResource wouldn't be so bad.

So, it's up to you and whatever others think is best.

@TheNormalnij
Copy link
Member

We should probably add an argument ensure to restartResource (defaulting to false)?

Why do you need that when you can get the state of your resource before calling? A new argument doesn't improve readability

@TheNormalnij
Copy link
Member

I suggest to remove a new 'ensure' command and just change 'restart' behavior. Current 'restart' behavior is disappointing.

@Lpsd
Copy link
Member

Lpsd commented Feb 14, 2025

I suggest to remove a new 'ensure' command and just change 'restart' behavior. Current 'restart' behavior is disappointing.

I agree with this too, but would we want to change the behaviour of restartResource function by default?

Technically that isn't backwards compatible, someone may rely on the fact a resource must be running when this function or command is called.

Nonetheless, it would be much better behaviour for the restart command and function to start resources that aren't running, instead of doing nothing/returning false.

@Fernando-A-Rocha
Copy link
Contributor

Changing restart command functionality to support starting a resource even if it is stopped does not make sense, as the name implies a stop then a start, not just a start.

New ensure command makes sense, it's practical. I support it.

@Nico8340
Copy link
Contributor Author

We should probably add an argument ensure to restartResource (defaulting to false)?

The reason I don't suggest adding a separate API function for this (e.g: ensureResource), is because we'd need to repeat the arguments and associated logic for those anyway.

Edit: since you'd need to upgrade restartResource to the new arg parser anyway, the code would actually end up being reasonably sized; having an additional function such as ensureResource wouldn't be so bad.

So, it's up to you and whatever others think is best.

I guess we can discuss that later, but in a separate issue or pull request, this one is focused solely on improving the commands

@Nico8340
Copy link
Contributor Author

Changing restart command functionality to support starting a resource even if it is stopped does not make sense, as the name implies a stop then a start, not just a start.

New ensure command makes sense, it's practical. I support it.

I want to introduce the new command because restart is well known and widely used, and I don't want to change that. ensure is also common in other game engine like multiplayer modifications, so it should feel familiar to developers interested in modern stuff

@TheNormalnij
Copy link
Member

I want to introduce the new command because restart is well known and widely used, and I don't want to change that. ensure is also common in other game engine like multiplayer modifications, so it should feel familiar to developers interested in modern stuff

As far as I know, only FiveM has the ensure command. The suggested restart behavior is much more common in Linux, e.g. the service command works in this way. I expect that the restart command starts stopped services and resources.
I can't imagine that somebody wouldn't expect to start a resource using the restart command.

I see no reason to have two commands with almost identical behavior instead of using one command with expected name.

Technically that isn't backwards compatible, someone may rely on the fact a resource must be running when this function or command is called.

I can't imagine a scenario when you need that. I would suggest writing a function that implements the old behavior if someone needs that. A failed restart commad forced me to use start in all my cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add resource server commands
4 participants