Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Don't put the servername in the filename of the default logging conf #13696

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Don't put the servername in the filename of the default logging conf #13696

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ __pycache__/
/*.log
/*.log.*
/*.log.config
/log.config
/*.pid
/.python-version
/*.signing.key
Expand Down
5 changes: 1 addition & 4 deletions debian/build_virtualenv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,6 @@ esac
# tweak the pid file location
/^pid_file:/ and s#:.*#: "/var/run/matrix-synapse.pid"#;

# tweak the path to the log config
/^log_config:/ and s/SERVERNAME\.log\.config/log.yaml/;

# tweak the path to the media store
/^media_store_path:/ and s#/media_store#/media#;

Expand All @@ -112,7 +109,7 @@ esac

# build the log config file
"${TARGET_PYTHON}" "${VIRTUALENV_DIR}/bin/generate_log_config" \
--output-file="${PACKAGE_BUILD_DIR}/etc/matrix-synapse/log.yaml"
--output-file="${PACKAGE_BUILD_DIR}/etc/matrix-synapse/log.config"
Copy link
Member

Choose a reason for hiding this comment

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

If you're renaming a conffile in debian packaging, you need to add stuff to the pre/postinst to help users.

See https://manpages.debian.org/unstable/dpkg/dpkg-maintscript-helper.1.en.html for more.

Copy link
Member

Choose a reason for hiding this comment

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

Also, if it's a yaml file, I think it should be called *.yaml. Your concern that synapse might treat it as a regular config file if you put it in the config directory is noted... so don't do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you're renaming a conffile in debian packaging, you need to add stuff to the pre/postinst to help users.

Will look into implementing a helper for that.

Also, if it's a yaml file, I think it should be called *.yaml. Your concern that synapse might treat it as a regular config file if you put it in the config directory is noted... so don't do that?

I think that's a foot gun I'd rather not implement. I'm sure that's an edge case, but since --config-path= can take directories and the config directory defaults to the directory of the last found config file I could foresee situations where (missing) config files are generated in the same directory where --config-path= is pointed to.


# add a dependency on the right version of python to substvars.
PYPKG=$(basename "$SNAKE")
Expand Down
6 changes: 2 additions & 4 deletions synapse/config/logger.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,10 +147,8 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
self.log_config = self.abspath(config.get("log_config"))
self.no_redirect_stdio = config.get("no_redirect_stdio", False)

def generate_config_section(
self, config_dir_path: str, server_name: str, **kwargs: Any
) -> str:
log_config = os.path.join(config_dir_path, server_name + ".log.config")
def generate_config_section(self, config_dir_path: str, **kwargs: Any) -> str:
log_config = os.path.join(config_dir_path, "log.config")
return (
"""\
log_config: "%(log_config)s"
Expand Down