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

Add --log-config cmd line parameter to stator #674

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

osmaa
Copy link
Contributor

@osmaa osmaa commented Dec 1, 2023

ref ab3648e#commitcomment-132861912

With this, it's possible to use the same logging config ini file for both the web and stator processes. Sample logging config included.

@andrewgodwin
Copy link
Member

Why introduce an ini file in the docker directory but not use it anywhere?

@osmaa
Copy link
Contributor Author

osmaa commented Dec 4, 2023

because there is no start script provided? Where should the config file be?

@andrewgodwin
Copy link
Member

Well, I more mean that it's odd to check a default in but not have it used; you could either make it an example one not in the docker directory, place it next to the code and have it as a relative-path default, or include it in the documentation.

@osmaa
Copy link
Contributor Author

osmaa commented Dec 5, 2023

Personally, I mount the config into the running container and then refer to it on the starting command line, because that allows me to tweak the config without rebuilding the image. That probably isn't what everyone wants to do, though..

@andrewgodwin
Copy link
Member

Yeah, my deployment infra assumes that a redeploy means a new image. It's mostly a matter of personal taste I suspect.

@AstraLuma
Copy link
Contributor

So what changes do you want to see, @andrewgodwin ?

@andrewgodwin
Copy link
Member

Well, if there's going to be a default config added to the docker image, I think the image should be set up to use that via environment variable or something.

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.

3 participants