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

Docker Hub Support #19

Closed
wants to merge 7 commits into from
Closed

Docker Hub Support #19

wants to merge 7 commits into from

Conversation

dreulavelle
Copy link
Member

@dreulavelle dreulavelle commented Dec 14, 2023

@dreulavelle
Copy link
Member Author

Alright, this is ready for review.

@dreulavelle dreulavelle self-assigned this Dec 14, 2023
```

> [!WARNING]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arent settings copied from default settings.json?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but you can't bind mount a file that isn't there. And if you bind mount to an empty file, it creates issues.

Copy link
Member Author

@dreulavelle dreulavelle Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The best way arround this is to bind to a directory.. so if we toss settings.json into a settings/settings.json and we binded to settings dir. Then it would work fine.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah move settings.json to data folder

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data, logs, and settings.json

Should be in a folder together I think? then we can just bind that 1 directory:
- ./config:/config

Or something to that effect..

@dreulavelle
Copy link
Member Author

There is one issue with this PR. The backend api isn't exposed. So only the front end (since its inside the container) can see the api for backend. If we want to see the backend api as well, I'll need to expose it in the Dockerfile and then add the port to the compose.

Is this something we care about? @Gaisberg @AyushSehrawat

@Gaisberg
Copy link
Collaborator

There is one issue with this PR. The backend api isn't exposed. So only the front end (since its inside the container) can see the api for backend. If we want to see the backend api as well, I'll need to expose it in the Dockerfile and then add the port to the compose.

Is this something we care about? @Gaisberg @AyushSehrawat

End user doesnt need the backend api exposed

@dreulavelle dreulavelle deleted the add_docker_support branch December 14, 2023 19:57
@dreulavelle
Copy link
Member Author

Recreated in another branch. Deleting/Ending this PR.

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.

2 participants