Skip to content
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 local docker dev server usage #1631

Merged
merged 2 commits into from
May 15, 2023
Merged

Fix local docker dev server usage #1631

merged 2 commits into from
May 15, 2023

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented May 12, 2023

Hopefully closes #1589

@ml-evs ml-evs requested review from CasperWA and JPBergsma as code owners May 12, 2023 15:09
@codecov
Copy link

codecov bot commented May 12, 2023

Codecov Report

Merging #1631 (3ef933b) into master (fa8325d) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1631   +/-   ##
=======================================
  Coverage   91.04%   91.04%           
=======================================
  Files          74       74           
  Lines        4578     4578           
=======================================
  Hits         4168     4168           
  Misses        410      410           
Flag Coverage Δ
project 91.04% <ø> (ø)
validator 90.93% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@JPBergsma
Copy link
Contributor

I have tried to build a docker container based on these changes.
When I run it with publish 8080:5000 it seems to work and the server is reachable under http://localhost:8080/v1/structures.
The line from your docker script still lists the port as 5000. I am not sure whether this is the port number you want to display here, as this port number is already reported by uvicorn as well.

@ml-evs
Copy link
Member Author

ml-evs commented May 15, 2023

I have tried to build a docker container based on these changes.
When I run it with publish 8080:5000 it seems to work and the server is reachable under http://localhost:8080/v1/structures.
The line from your docker script still lists the port as 5000. I am not sure whether this is the port number you want to display here, as this port number is already reported by uvicorn as well.

I get what you're saying but I think that's fine, and documented. There's no (simple) way for the app to know it is running within docker and it follows the vague 80/8080 port convention for prod/Dev servers.

If this overall works for both of us then I think I will merge it!

@ml-evs ml-evs merged commit 2062a20 into master May 15, 2023
@ml-evs ml-evs deleted the ml-evs/fix_docker_local branch May 15, 2023 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Local version app not reachable in docker container.
2 participants