Skip to content

Delete redundant service verb#406

Merged
mjcheetham merged 1 commit intomicrosoft:mainfrom
mjcheetham:cleanup2
Jul 16, 2020
Merged

Delete redundant service verb#406
mjcheetham merged 1 commit intomicrosoft:mainfrom
mjcheetham:cleanup2

Conversation

@mjcheetham
Copy link
Member

@mjcheetham mjcheetham commented Jul 15, 2020

Delete the ServiceVerb which is now redundant; the ListVerb performs the exact same actions.

Copy link
Contributor

@derrickstolee derrickstolee left a comment

Choose a reason for hiding this comment

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

A quick git grep service found an interesting case hat might cause some problems:

Scalar.Installer.Windows/Setup.iss:  Exec('scalar.exe', 'service --help', '', SW_HIDE, ewWaitUntilTerminated, ResultCode);

Likely this is a simple replacement, like s/service --help/list --help/.

@mjcheetham
Copy link
Member Author

A quick git grep service found an interesting case hat might cause some problems:

Scalar.Installer.Windows/Setup.iss:  Exec('scalar.exe', 'service --help', '', SW_HIDE, ewWaitUntilTerminated, ResultCode);

Likely this is a simple replacement, like s/service --help/list --help/.

The "TODO" above this is interesting:

procedure StopMaintenanceTasks();
var
  ResultCode: integer;
begin
  // TODO: #185 Instead of calling --help, use the correct action for stopping the
  // maintenance task
  Exec('scalar.exe', 'service --help', '', SW_HIDE, ewWaitUntilTerminated, ResultCode);
end;

That issue (#185) was closed by @derrickstolee saying:

Should not be needed with #236. Closing.

..but looking at #236, it appears this was closed without a code change. You said:

If we run the maintenance tasks in process, then we cannot authenticate with the remote. This makes the fetch-commits-and-trees step impossible and the commit-graph step likely to fail.

Do we still need to implement this TODO and stop maintenance tasks? Don't we really just need a new verb scalar stop [--force/confirm etc] that will stop existing tasks?

I can replace this with a dummy command again for now, but feels like perhaps #185 should be reopened, no?

@derrickstolee
Copy link
Contributor

I can replace this with a dummy command again for now, but feels like perhaps #185 should be reopened, no?

I agree with your investigation and conclusion. Thanks!

Delete the ServiceVerb which is now redundant; the ListVerb performs the
exact same actions. Also delete unit tests for this verb.
@mjcheetham
Copy link
Member Author

I can replace this with a dummy command again for now, but feels like perhaps #185 should be reopened, no?

I agree with your investigation and conclusion. Thanks!

Reopened the issue: #185 (comment)

@mjcheetham mjcheetham force-pushed the cleanup2 branch 2 times, most recently from c075657 to bb1ae4a Compare July 15, 2020 16:49
@mjcheetham mjcheetham merged commit 83edf26 into microsoft:main Jul 16, 2020
@mjcheetham mjcheetham deleted the cleanup2 branch July 16, 2020 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants