Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Preloading Modules / Dockerization of FAME / Dockerception (combined PR of all changes, do not merge!) #67
base: master
Are you sure you want to change the base?
Preloading Modules / Dockerization of FAME / Dockerception (combined PR of all changes, do not merge!) #67
Changes from 27 commits
06da9ab
4861433
3340d70
d2984a0
27ece3c
caa615f
2366265
56b5056
605db70
0c23514
4b81f81
a55a41a
b522d5d
1e3f93a
ebc2f69
9ecbb91
055ff83
d98ca75
891643a
4c2bef1
4ffc0c3
c0fbe59
dce2691
51c7396
1d4ae7a
56c6854
d71893b
d231c59
b734ab3
c4342c3
a794a88
361f9cb
13469d4
3bc582f
8547eda
0ada515
c6cc72d
5cf8db2
97f4f4b
e2eb1e1
11a0615
59ee23a
97a1743
635f6ba
676a76f
d8d3c4d
26691dc
680b31a
9ceb492
fc4a928
ebdbd7d
93412e0
e6de6a1
a26b121
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
How are the username and password set ?
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.
Shouldn't these steps be in the Dockerfile ?
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.
pip yes, install_docker no. install_docker requires a running database which is not the case during the Docker build.
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.
Shouldn't this step be in the Dockerfile ?
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 step should not be in the Dockerfile in my opinion, because there are several volumes that are mounted beneath /fame that may have the wrong permissions for FAME to access it. Thus, we do the
chown
at runtime.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.
These two steps look like they should be in the Dockerfile
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.
setting up git, yes. But I am not sure about the temp dir. Sure, we can create it in the Dockerfile but the chown is still necessary here.
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.
Given that you are using a dedicated MongoDB instance for FAME and that it is only available to the dedicated Docker network, do we really need to bother with this form of authentication ? I think it would probably be simpler to use
MONGO_INITDB_ROOT_USERNAME
andMONGO_INITDB_ROOT_PASSWORD
directly in thedocker-compose.yml
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.
In general, you are perfectly right. But the main drawback I saw in that (and therefore chose not to use it) is that
MONGO_INITDB_ROOT_USERNAME
creates a root user that has full access rights to everything in the MongoDB. Using the adduser script I was able to create a user that has only access to the FAME databases.You are right that this should have no impact on a dedicated instance for FAME, it becomes only relevant if the MongoDB is a shared instance. I leave that decision up to you.