-
Notifications
You must be signed in to change notification settings - Fork 629
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
Added docker support for debugging #2298
base: master
Are you sure you want to change the base?
Conversation
@mk0sojo That's awesome work. Hopefully this will make it much easier for people to get started. |
@jonparker Still needs to add a better debugging experience and need to speed up the restore / build process a bit. |
@jonparker @MisterJames Added support for debugging in VS Code and tried to speed up the build time a bit.
The only (known) limitation is that it is not possible to edit an HTML or CSS file without rebuilding. Let me know what you think. |
That's great. Have you heard of docker-sync? It allows for you to build within the container while having the files checked out on the host. |
It looks like it could be usefull, my only concern is that it looks a bit complicated to install (on windows in particular https://github.com/EugenMayer/docker-sync/wiki/docker-sync-on-Windows). I'm not sure if that is any better than the installation instructions we currently have running everything locally? On our codathons, is there any particular step that users find difficult or is it just the number of steps? |
@mk0sojo I tested this on Windows and there was an issue with the line endings in
The cause was the bash script being checked with the Windows line-ending, The issue is described is more detail here: The solution is to configure I've made this change to a clone of your branch here: I also rewrote the Docker connection string logic. Instead of using the Let me know if and how you want to merge these changes. |
@tony-ho Thanks for the feedback and testing it on Windows. The changes to the connection string logic looks great as well. Let me know if you have any more feedback. |
This is great stuff. I'd like to merge this in, do we need another commit @mk0sojo for those line endings? |
@MisterJames I have added @tony-ho changes to the pull request so it should be ok to merge now. |
@MisterJames Is there anything else missing here before it can be 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.
@mk0sojo
I realize this is a bit old and seems forgotten by the maintainers. Still:
I've been working on similar idea for the past couple of days. Then this PR. I think some value could be added by tweaking some of the changes. Please see comments. I could do the changes but wanted to get your feedback before that.
@@ -0,0 +1,12 @@ | |||
FROM microsoft/aspnetcore-build:2.0 |
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.
@mk0sojo This whole dockerfile seems to do nothing more than just install the vs debugger and then on startup just keep the process running. What purpose does it serve?
RUN gulp clean && gulp min | ||
EXPOSE 80/tcp | ||
RUN chmod +x ../../entrypoint.sh | ||
CMD /bin/bash ../../entrypoint.sh |
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.
CMD /bin/bash ../../entrypoint.sh | |
CMD /bin/bash "dotnet ef database update && dotnet AllReady.dll" |
Instead of having a whole separate file (entrypoint.sh
) I believe we could just simply chain the migration and the app start. That we we can drop the .sh file and also the end-of-line issues discussed in the PR comments.
@@ -169,7 +169,7 @@ public IServiceProvider ConfigureServices(IServiceCollection services) | |||
|
|||
//Hangfire | |||
services.AddHangfire(configuration => configuration.UseSqlServerStorage(Configuration["Data:HangfireConnection:ConnectionString"])); | |||
|
|||
services.AddScoped<IAllReadyUserManager, AllReadyUserManager>(); |
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 file seems to only have whitespace changes - perhaps it could be excluded/rolled-back from the PR (i.e. github checkout origin/master -- AllReadyApp/Web-App/AllReady/Startup.cs
)
ports: | ||
- "8000:80" | ||
depends_on: | ||
- db |
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 the db container takes a bit of time to spin up, I'd suggest adding
restart: on-failure
So that docker could retry starting the app if the db is not yet up and running.
ACCEPT_EULA: "Y" | ||
|
||
volumes: | ||
sql-server-data: |
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.
Also I could not see where is the logic that actually creates the database table AllReady
like done in the appveyor run script
Lines 46 to 47 in 37833fc
- ps: cd (Join-Path $env:APPVEYOR_BUILD_FOLDER "AllReadyApp\Web-App\AllReady") | |
- ps: sqlcmd -S "(local)\SQL2016" -Q "Use [master]; CREATE DATABASE [AllReady]" |
>&2 echo "SQL Server is starting up" | ||
sleep 1 | ||
done | ||
|
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 file seems to only run the migration and does not actually start the app. i.e. dotnet AllReady.dll
is nowhere to be seen...
For #2247
First shot at debugging the web app in a docker containers (one for the app and one for SQL server). This is the steps to try this out:
The main remaining issue is after changing anything in the project the whole project is restored, build and nom packages are installed (even if it is just a change to a HTML file)
Will try to solve it, but please let me know what you think so far. I have only tried it on a Mac so far but it “should” work on Windows as well.