-
Notifications
You must be signed in to change notification settings - Fork 520
Add start stop button on ServiceDetails page #948
base: main
Are you sure you want to change the base?
Conversation
@glennc does this conflict with your changes? |
For your specific scenario, you can have VS Code attach to the process Tye is already running. The only time that wouldn't work is if you are debugging something that runs in startup before steadystate hits. Is there some reason that doesn't work for you? Not that this is a bad idea or PR, I am still happy to have it added. I was thinking of putting it on the main page for every row as you listed in your alternative (that's what my quick hackup of this does). I kind of like the idea of being able to do both, i.e. the button you have in your gif can stop the specific replica you have selected in the replica dropdown while the index page button would stop all replicas. Anyone have thoughts on that? I haven't looked at the code for this yet, but I assume this PR would do anything my own would and I'm happy to work through it until it supersedes it. |
Thanks for your feedback @glennc ! I would add to my scenario the case where you want to also modify the code, so you can run it outside tye, and test with all the other services still running with tye. At the moment start/stop button does not respect replica dropdown, mostly since I haven't really used replica functionality. I can definitely move the button to the index page (and keep it on the details page?) Use icons or just start/stop? Any preference on colors etc? I would really like to get this in, so just let me know what the next step would be. |
Reopen after messing with master/main and merging in latest changes |
I don't want to feature creep here but now I'm thinking we should have both graceful and non-graceful shutdown |
Hey guys! |
src/Microsoft.Tye.Hosting/TyeHost.cs
Outdated
services.AddScoped(sp => | ||
{ | ||
var server = sp.GetRequiredService<AspNetCore.Hosting.Server.IServer>(); | ||
var addressFeature = server.Features.Get<AspNetCore.Hosting.Server.Features.IServerAddressesFeature>(); | ||
string baseAddress = addressFeature.Addresses.First(); | ||
return new System.Net.Http.HttpClient { BaseAddress = new Uri(baseAddress) }; | ||
}); |
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.
Feels slightly overkill to make requests to the API. We can just call the function, right?
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 think I'll need some help here... how do I access ProcessRunner from razor page?
var api = new TyeDashboardApi(); | ||
|
||
app.UseEndpoints(endpoints => | ||
if (_logger != null && _replicaRegistry != null) |
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.
How would these be null?
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.
Compiler shows null warnings
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 am not sure if it makes a lot of difference to delay creation of _logger
and _replicaRegistry
to when StartAsync()
is called. I would consider making them non-nullable and create/have them injected in the constructor, which would enable using them here without a null check.
Thanks for all the work. Would be nice to have this feature merged. : ) |
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.
Thank you for your submission! I made a few suggestions, please take a look--thx!
var api = new TyeDashboardApi(); | ||
|
||
app.UseEndpoints(endpoints => | ||
if (_logger != null && _replicaRegistry != null) |
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 am not sure if it makes a lot of difference to delay creation of _logger
and _replicaRegistry
to when StartAsync()
is called. I would consider making them non-nullable and create/have them injected in the constructor, which would enable using them here without a null check.
@@ -204,6 +204,13 @@ private IHost BuildWebApplication(Application application, HostOptions options, | |||
}); | |||
}); | |||
services.AddSingleton(application); | |||
services.AddScoped(sp => |
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.
Not sure if this is necessary. You should be able to access HttpContext.Request
from Razor pages/views: https://learn.microsoft.com/en-us/aspnet/core/fundamentals/http-context?view=aspnetcore-7.0#access-httpcontext-from-razor-pages and build the URL to the start/stop API request from that.
@@ -1,5 +1,6 @@ | |||
@page "/services/{ServiceName}" | |||
@inject Application application | |||
@inject HttpClient Http |
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.
See my other comment regarding HttpClient
injection. Can you try to see if you can get away with creating a HttpClient
instance directly?
@@ -178,7 +178,7 @@ private async Task BuildAndRunProjects(Application application) | |||
} | |||
} | |||
|
|||
private void LaunchService(Application application, Service service) | |||
public void LaunchService(Application application, Service service) |
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.
public void LaunchService(Application application, Service service) | |
internal void LaunchService(Application application, Service service) |
@@ -460,7 +460,7 @@ async Task RunApplicationAsync(IEnumerable<(int ExternalPort, int Port, string? | |||
service.Items[typeof(ProcessInfo)] = processInfo; | |||
} | |||
|
|||
private Task KillRunningProcesses(IDictionary<string, Service> services) | |||
public Task KillRunningProcesses(IDictionary<string, Service> services) |
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.
public Task KillRunningProcesses(IDictionary<string, Service> services) | |
internal Task KillRunningProcesses(IDictionary<string, Service> services) |
var app = context.RequestServices.GetRequiredService<Application>(); | ||
|
||
var name = (string?)context.Request.RouteValues["name"]; | ||
if (!string.IsNullOrEmpty(name) && app.Services.TryGetValue(name, out var service)) |
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.
Again, if no service with given name is found, 404 and no redirect, please.
|
||
context.Response.Redirect($"/services/{name}"); | ||
|
||
return; |
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.
Redundant return
, please remove.
Yes, sorry, we (MS) are not developing Tye actively at this moment. I made a few suggestions, hope this helps. |
This PR adds Start/Stop button on ServiceDetails page to enable starting and stopping service on demand.
This is motivated by the following scenario:
I have several dotnet applications running locally, and I might want to debug one of them using visual studio (code). To do this now, I would need to stop tye, comment out the service I want to debug, run tye again, then run my application with debugger.
Alternative is to add the button on the Index page in every row.
Related to #876
Thanks to @dillenmeister for the initial idea.