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

Conversation

behrmann
Copy link
Contributor

@behrmann behrmann commented Sep 1, 2022

Pull Request Checklist

As just touched on in #13669. This is probably still missing something and I haven't tested this, but I wanted to open it as a sort of todo.

/cc @richvdh

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

@behrmann behrmann requested a review from a team as a code owner September 1, 2022 13:24
@@ -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.

@clokep
Copy link
Member

clokep commented Oct 19, 2022

@behrmann Do you have interest in finishing this up?

@behrmann
Copy link
Contributor Author

Yes, sorry about the delay. Been swamped with other work. This is at the top of my synapse todo list for what it's worth. Will try to get back to it asap.

@behrmann behrmann mentioned this pull request Jan 25, 2023
4 tasks
@DMRobertson DMRobertson added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Feb 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants