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
Fix 404 error #17
Fix 404 error #17
Conversation
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 looks to be a good, more robust solution to the problem. I've flagged a couple of minor changes to make.
DeployClient/Program.cs
Outdated
TimeSpan interval = new TimeSpan(0, 0, 0, 2); | ||
Dictionary<string, dynamic> response; | ||
var successfullyReachedApi = false; | ||
DateTime apiNotFoundAbortTime = DateTime.Now.AddSeconds(Options.InstallationStatusTimeout?? 60); |
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 please improve the whitespace formatting around the null conditional operator.
DeployClient/Program.cs
Outdated
// The api should not be returning a 404 status at this point | ||
if (!success) | ||
{ | ||
throw new Exception("Remote API returned status 404"); |
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 can we throw something more specific than System.Exception
. It's bad practise to throw these. I think System.Web.HttpException
would be more appropriate instead.
Change to how checking the installation status works after starting an installation. Currently, on large installations, it can attempt to get the status before the web api has been fully created.
Api.GetSession()
. If the time limit is reached, then abort from the process. If it successfully manages to connect with the api before that time, then carry on as usual