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.

Sign up for GitHub

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

Jump to bottom

[MLDB-2086] passing the config to mldb_runner in debug and release #775

Merged
merged 3 commits into from Dec 9, 2016
Merged

[MLDB-2086] passing the config to mldb_runner in debug and release #775

merged 3 commits into from Dec 9, 2016

Conversation

guyd
Copy link
Contributor

@guyd guyd commented Dec 9, 2016

  • passing the mldb.conf when doing make to allow logging
  • storing the mldb.conf in the docker image and passing it to mldb_runner
  • fix a crash when failing to parse the command line

jraby
jraby reviewed Dec 9, 2016
View reviewed changes
templated_files.mk
@@ -10,6 +10,7 @@ $(eval $(call mldb_install_templated_directory,mldb/container_files/public_html/

$(eval $(call mldb_install_templated_file,mldb/container_files/init/05-mldb-id-mapping.sh,$(ETC)/my_init.d/05-mldb-id-mapping.sh,555))
$(eval $(call mldb_install_templated_file,mldb/container_files/init/mldb_runner.sh,$(ETC)/service/mldb_runner/run,555))
$(eval $(call mldb_install_templated_file,mldb/container_files/mldb.conf,$(ETC)/service/mldb_runner/run,664))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could put mldb.conf directly in /etc instead of deep inside the runit directory.

Minor nit: 0644.

@jraby
Copy link
Contributor

jraby commented Dec 9, 2016

I'm not sure if we should overload MLDB_EXTRA_FLAGS for this purpose.
mldb.conf might deserve its own template variable since we probably want to pass it all the time.

I see EXTRA_FLAGS as the hook which can be used to pass image specific parameters.
With this change, we would have to remember to set the config file along with the extra flags we want to pass.

I think we could add --config-path=$(MLDB_CONFIG_FILE) to mldb_runner.sh and set the path directly in TEST_$(1)_RAW_COMMAND (or change MLDB_CONFIG_FILE when doing a debug build)

guyd
guyd commented Dec 9, 2016
View reviewed changes
container_files/init/mldb_runner.sh
@@ -20,5 +20,6 @@ exec /sbin/setuser _mldb \
--plugin-directory {{MLDB_GLOBAL_PLUGINS_PATH}} \
--plugin-directory {{MLDB_LOCAL_PLUGINS_PATH}} \
--http-base-url "${HTTP_BASE_URL}" \
--config-path=/etc/mldb.conf \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jraby Est-ce que je devrais utiliser une variable d'environnement ici?

@guyd guyd merged commit 9ffe910 into master Dec 9, 2016
@guyd guyd deleted the pass_config branch December 12, 2016 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jraby jraby jraby left review comments

Assignees

@jraby jraby

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@guyd @jraby