Closed ajinkyababar closed 2 years ago
ajinkyababar
commented
2 years ago I did a quick pass-through and tried to avoid making too make comments all at once. The biggest thing I noticed without diving too deep was the need to split up the InitalizeRouter logic into a type constructor that builds up an HTTP server configuration, and a method that's responsible for running that server and handling signal interrupts.
I have started working on review comments will raise PR once done
ajinkyababar
commented
2 years ago It looks like github is complaining about merge conflicts. Can we fix that up locally and push up those changes before I review further?
On my end, it is not showing any conflicts. it says - This branch has no conflicts with the base branch
timflannagan
commented
2 years ago Oh it looks like the github UI is complaining about the merge commit present in the set of commits? Can we get rid of that commit locally by rebasing against the main branch? It should just be something like the following: git pull --rebase upstream main assuming that upstream is the remote name that points to this repository.
timflannagan
commented
2 years ago /approve /lgtm
sesheta
commented
2 years ago [APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: timflannagan
The full list of commands accepted by this bot can be found here.
The pull request process is described here
I did a quick pass-through and tried to avoid making too make comments all at once. The biggest thing I noticed without diving too deep was the need to split up the InitalizeRouter logic into a type constructor that builds up an HTTP server configuration, and a method that's responsible for running that server and handling signal interrupts.