Skip to content

Conversation

@fhofmann
Copy link

Added a parameter to control the startImmediately behaviour of the win32 service when installing the service.
This is useful when installing, then configuring something, and then starting the service after this configuration step is done.
Default is the same as before.

@dotnetcraig
Copy link

How weird. I've just been working on something to try and achieve the same outcome.

My solution was just adding another action so you would do "MyService action:delayedinstall" to get it to just install the service. I see you've gone down the parameter route.

For yours, would you have to do "MyService action:install startimmediately:true" ?

@dotnetcraig dotnetcraig mentioned this pull request Nov 16, 2017
@fhofmann
Copy link
Author

Yeah, true would be the default, so action:install startimmediately:false would be the same as your action:delayedinstall.
One more thing I thought about, the service will still be set to start automatically (for instance when restarting the system - i think).
That could be another parameter, but at least for our use case this isn't needed.

@PeterKottas
Copy link
Owner

Hi guys, just looking at this now. It definitely should be a part of this lib if you found a sensible use-case for it. It's just a matter of choosing the best syntax for it as both this and #56 have pretty much the same functionality. I am slightly inclining towards the solution by @avisonmaher as it seems a bit more verbose and won't require and extra parameter. What do you think guys? Any reasons come to mind why we should use one or another?

@fhofmann
Copy link
Author

Hey, when I implemented this, I just looked at the underlying calls to the service and saw that there was an extra paramerter, that was always set to true, so I added one to forward this functionality. I don't really see a reason to favor one over the other. They both provide the same functionality, "delayedinstall" is maybe not the best name, but tbh I can't really think of a better one.

@dotnetcraig
Copy link

I don't mind, I'd agree "delayedinstall" isn't the best name. Taking inspiration from https://ss64.com/nt/sc.html it could be changed to "create".
I think @fhofmann 's solution is a quicker and tidier way right now but does require typing more to get the service installed. I've created a new option on an existing parameter however it has created duplicated code so probably requires a bit more work to tidy it up with influences from #55 - which I've been thinking about but haven't gotten too.

Copy link
Owner

@PeterKottas PeterKottas left a comment

Choose a reason for hiding this comment

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

Just a few small things


public string DisplayName { get; set; }

public bool? StartImmediately { get; set; }
Copy link
Owner

Choose a reason for hiding this comment

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

Could you please change this to not nullable and add default property value?

cred,
autoStart: true,
startImmediately: true,
startImmediately: config.StartImmediately ?? true,
Copy link
Owner

Choose a reason for hiding this comment

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

if it's nullable it should be config.StartImmediately.HasValue ? config.StartImmediately.Value : true instead. But I think it's better to make it non nullable like mentioned in my previous comment.

ConfigureService(innerConfig);

return 0;
return 0;
Copy link
Owner

Choose a reason for hiding this comment

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

There seems to be a lot of false positive diffs because of formatting changes. Just some extra white spaces I guess. Could you please reformat this doc?

@PeterKottas
Copy link
Owner

Ok I'll be merging this one in guys. Just a few small issues with the code so please @fhofmann could you take a look? I'll be merging in and releasing right after this is done.

@fhofmann
Copy link
Author

Thanks for the review.
Okay, should be done. Do I have to do a new pull request or do you see the changes automatically?
Cheers
Frieder

@PeterKottas PeterKottas merged commit a58da48 into PeterKottas:master Nov 27, 2017
@PeterKottas
Copy link
Owner

Cheers mate, all good. There's still some formatting issues even after this commit but screw that. I'll fix those myself and this PR is ok to go in as far as I am concerned.

@fhofmann fhofmann deleted the parameter-startimmediately branch November 28, 2017 16:39
@dotnetcraig
Copy link

I've tried this by adding both:

MyWindowsService startimmediately:false action:install (as discussed in here)
MyWindowsService start-immediately:false action:install (as referenced on the documentation)

and tried swapping the parameters around:

MyWindowsService action:install startimmediately:false
MyWindowsService action:install start-immediately:false

But it isn't working, it is installing the service and running it straight away.

@fhofmann
Copy link
Author

For me it works fine:

action:install start-immediately:true
-> service running
action:install start-immediately:false
-> service is there, but is stopped.

@fhofmann
Copy link
Author

One thing I noticed, the message being displayed might be miss leading when using action:install and start-imediately:false

"Successfully registered and started service" will be written to the console.

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.

3 participants