GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Remove '-ComputerName' from 'Get/Set/Remove-Service' by daxian-dbw · Pull Request #5094 · PowerShell/PowerShell

Conversation

@daxian-dbw

@daxian-dbw Dongbo Wang (daxian-dbw) commented Oct 12, 2017

Copy link
Copy Markdown
Member

Fix #5090
Remove '-ComputerName' from 'Get/Set/Remove-Service'

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Dongbo Wang (daxian-dbw) added 2 commits October 12, 2017 11:18
@daxian-dbw
Remove '-ComputerName' parameter for 'Get/Set/Remove-Service'
b671e0e
@daxian-dbw
[Feature] Minor fix
53a4083
@daxian-dbw Dongbo Wang (daxian-dbw) assigned Ilya (iSazonov) Oct 12, 2017
@daxian-dbw
Dongbo Wang (daxian-dbw) requested review from Steve Lee (SteveL-MSFT), Aditya Patwardhan (adityapatwardhan) and Andrew (anmenaga) October 12, 2017 18:29
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Oct 12, 2017

Copy link
Copy Markdown
Member Author

It's easier to review the changes after ignoring whitespaces: https://github.com/PowerShell/PowerShell/pull/5094/files?w=1
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@daxian-dbw
[Feature] Update tests
cc71072
anmenaga
Andrew (anmenaga) suggested changes Oct 12, 2017
View reviewed changes

@anmenaga Andrew (anmenaga) left a comment

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
anmenaga
Andrew (anmenaga) approved these changes Oct 12, 2017
View reviewed changes
adityapatwardhan
Aditya Patwardhan (adityapatwardhan) requested changes Oct 12, 2017
View reviewed changes
Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/Service.cs Outdated
NativeMethods.SERVICE_CONFIG_DESCRIPTION,
buffer);
// If '-Status' parameter is specified, do the necessary action to bring about the desired result

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Oct 12, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I do not like this comment. It says if -Status, do something to get what you want. Can we be more specific or remove this comment.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Oct 12, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sure, will change it to // Handle '-Status' parameter.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/Service.cs
//stop service,give the force parameter always true as we have already checked for the dependent services
//Specify NoWait parameter as always false since we are not adding this switch to this cmdlet
DoStopService(service, true, true);
}

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Oct 12, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Since we are touching the code here, can we use named parameters?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Oct 12, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Will do.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/Service.cs
WriteObject(displayservice);
if (!service.Status.Equals(ServiceControllerStatus.Paused))
//pause service
DoPauseService(service);

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Oct 12, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This comment is unnecessary.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Oct 12, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I didn't change this one. I will remove it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/Service.cs
bool objServiceShouldBeDisposed = false;
try
if (InputObject != null)
{

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Oct 12, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This code looks very similar to line 1485. Can we refactor?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Oct 12, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

They are from two different cmdlets: Set-Service (line 1485) and Remove-Service (2083)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@daxian-dbw
Address comments
07d5acd
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Oct 12, 2017

Copy link
Copy Markdown
Member Author

Aditya Patwardhan (@adityapatwardhan) Your comments are addressed. Can you please take another look?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
adityapatwardhan
Aditya Patwardhan (adityapatwardhan) approved these changes Oct 12, 2017
View reviewed changes
iSazonov
Ilya (iSazonov) reviewed Oct 13, 2017
View reviewed changes
Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/Service.cs Outdated
ServiceCommandException exception =
new ServiceCommandException(message, innerException);
exception.ServiceName = serviceName;
(null == innerException) ? "" : innerException.Message);

@iSazonov Ilya (iSazonov) Oct 13, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can we use String.Empty?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Oct 13, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Will do.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Oct 13, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please clarify for me: I found many "*" strings in cs files - should we create a constant for it?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/Microsoft.PowerShell.Commands.Management/commands/management/Service.cs
foreach (ServiceController service in OneService(pattern))
ServiceController service = GetOneService(pattern);
if (service != null)
{

@iSazonov Ilya (iSazonov) Oct 13, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It seems GetOneService() never return null.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Oct 13, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

When the service doesn't exist, accessing sc.Status; will throw InvalidOperationException. In that case the method will catch the exception and return null.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Oct 13, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thanks!

Closed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@iSazonov Ilya (iSazonov) added Breaking-Change breaking change that may affect users Documentation Needed in this repo Documentation is needed in this repo labels Oct 13, 2017
@daxian-dbw
[Feature] Address some more comments
4d7b406
iSazonov
Ilya (iSazonov) approved these changes Oct 13, 2017
View reviewed changes
@iSazonov
Ilya (iSazonov) merged commit f2eaeb8 into PowerShell:master Oct 13, 2017
@daxian-dbw
Dongbo Wang (daxian-dbw) deleted the service branch October 13, 2017 18:19
This was referenced Oct 24, 2017
Improve error message for Get-Service with an invalid ComputerName #2747
Closed
Removed ComputerName and added Credential from the Set-Service docume… MicrosoftDocs/PowerShell-Docs#1805
Merged
@joandrsn Jonas B. Andersen (joandrsn) mentioned this pull request Oct 31, 2017
Removed computername from service commands MicrosoftDocs/PowerShell-Docs#1822
Merged
@DynConcepts David V. Corbin (DynConcepts) mentioned this pull request Dec 14, 2017
Remove-Service on other computers.. #5691
Closed
@joeyaiello Joey Aiello (joeyaiello) mentioned this pull request Oct 15, 2018
Docs needed for 'Remove '-ComputerName' from 'Get/Set/Remove-Service'' MicrosoftDocs/PowerShell-Docs#3075
Closed
@joeyaiello Joey Aiello (joeyaiello) removed Documentation Needed in this repo Documentation is needed in this repo labels Oct 15, 2018
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@daxian-dbw @iSazonov
Remove '-ComputerName' from 'Get/Set/Remove-Service' (PowerShell#5094)
46d60f9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) adityapatwardhan approved these changes
@iSazonov Ilya (iSazonov) iSazonov approved these changes
@SteveL-MSFT Steve Lee (SteveL-MSFT) Awaiting requested review from SteveL-MSFT
+1 more reviewer
@anmenaga Andrew (anmenaga) anmenaga approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@iSazonov Ilya (iSazonov)

Labels

Breaking-Change breaking change that may affect users

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@daxian-dbw @anmenaga @adityapatwardhan @iSazonov @joeyaiello
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.