Fix docker build script #213
Conversation
|
Thank you @Deliay. A rather large refactor is under way on the Thanks for the work on the docker script, I haven't really maintained it at all. I appreciate the contribution. |
|
Update: PR #211 is merged into main. I will be reviewing the changes here soon to see how it needs to change |
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "Urls": "http://localhost:57940", | |||
| "Urls": "http://0.0.0.0:57940", | |||
There was a problem hiding this comment.
Binding to 0.0.0.0 here is a security risk if the machine running NetPad is exposed to an untrusted network. Instead we can have the docker script create an appsettings.Local.json that overrides that setting and sets it to 0.0.0.0. NetPad will use a appsettings.Local.json file if one exists.
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "Urls": "http://localhost:57930", | |||
| "Urls": "http://0.0.0.0:57930", | |||
README.md
Outdated
| FROM netpad | ||
| RUN apk add dotnet6-sdk | ||
| RUN wget .... | bash -c | ||
| RUN ... |
There was a problem hiding this comment.
Lines 284 and 285 seem incomplete (ie. ...)?
| <Exec WorkingDirectory="$(SpaRoot)" Command="npm run build" /> | ||
| <Exec WorkingDirectory="$(SpaRoot)" Command="npm run build:ssr -- --prod" Condition=" '$(BuildServerSideRenderer)' == 'true' " /> | ||
| <Exec WorkingDirectory="$(SpaRoot)" Command="npm install" Condition="'$(SKIP_SPA_BUILD)' != 'true' "/> | ||
| <Exec WorkingDirectory="$(SpaRoot)" Command="npm run build" Condition=" '$(SKIP_SPA_BUILD)' != 'true' And '$(TARGET)' != 'web' " /> |
There was a problem hiding this comment.
Why do we not want this when TARGET is web?
There was a problem hiding this comment.
The frontend project has been built ahead in docker script, so we need to skip it when running the MSBuild target.
|
|
||
| if (!string.IsNullOrWhiteSpace(additionalArgs)) | ||
| args += " " + additionalArgs; | ||
| var args = $"{additionalArgs} {projectArgs}"; |
There was a problem hiding this comment.
The new change removes the "" wrapping the project path that is still needed.
|
@Deliay I requested some changes before merging the PR. In addition let's hold off on commit (Bump .NET to 8 and bump dependencies to latest version). The package upgrade has a little more to it, especially related to the swagger generated API client. For .NET 8 I want to run some tests on my end to make sure everything still works with that upgrade so I'll go ahead and update to .NET 8 in a later PR once I've validated it. Can you also revert the change to v8 in the README introduced in Lastly, the change that you introduced in (Support lanuch OmniSharp server via 'dotnet exec OmniSharp.dll') did you test this still works when running the app without docker? Really appreciate your contribution! |
|
@tareqimbasher would you be interested in another pr mainly for fixing the docker file? I have a a working dockerfile on my pc that uses a multistage build. |
|
@Jogai yes I would be interested. Thanks for the contribution! |
| <Exec WorkingDirectory="$(SpaRoot)" Command="npm run build" /> | ||
| <Exec WorkingDirectory="$(SpaRoot)" Command="npm run build:ssr -- --prod" Condition=" '$(BuildServerSideRenderer)' == 'true' " /> | ||
| <Exec WorkingDirectory="$(SpaRoot)" Command="npm install" Condition="'$(SKIP_SPA_BUILD)' != 'true' "/> | ||
| <Exec WorkingDirectory="$(SpaRoot)" Command="npm run build" Condition=" '$(SKIP_SPA_BUILD)' != 'true' And '$(TARGET)' != 'web' " /> |
There was a problem hiding this comment.
The frontend project has been built ahead in docker script, so we need to skip it when running the MSBuild target.
Deliay
changed the title
I tried to build a Docker image of NetPad, but it failed. I fixed the Dockerfile and then incidentally upgraded .NET to 8.0 and also upgraded most of the NetPad dependencies to the latest version.
This pull request includes these changes:
dotnet exec OmniSharp.dlldue to the container limitation