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

Create Logging Configuration Heirarchy #2535

Merged
merged 10 commits into from
Mar 8, 2020

Conversation

mark-monteiro
Copy link
Member

Separate logging configuration into logging.default.json and logging.user.json so that we can update logging.default.json at will in the future and allow users to manage their own logging.user.json file which overwrites the defaults.

Changes

  • This builds upon the PR Improve migrations so they are more maintainable #2523, which should be merged first
  • On startup, change the name of the config file that is created and loaded from logging.json to logging.default.json. The deprecated logging.json file will be left in place.
  • Add a migration to initialize the logging.user.json file. If the user has made modifications to logging.json, that is used to initialize the new file. Otherwise it is initialized with empty content.
  • Update the config loading to merge logging.default.json and logging.user.json (the user config file is optional)

Issues
Fixes #2511

@mark-monteiro mark-monteiro requested review from a team and removed request for a team March 7, 2020 17:17
@joshuaboniface joshuaboniface marked this pull request as ready for review March 8, 2020 02:01
Copy link
Member

@joshuaboniface joshuaboniface left a comment

Choose a reason for hiding this comment

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

Trusting that this all works 🤞

@joshuaboniface joshuaboniface added this to In progress in Release 10.5.0 via automation Mar 8, 2020
Also save migration configuration after each migration instead of at the end in case an exception is thrown part way through the list
Also use a list instead of an array to store executed migrations in the configuration class
@mark-monteiro
Copy link
Member Author

Okay, pushed some more updates:

  • Save migration configuration immediately after each one executes (instead of waiting until all have finished executing). In the case an exception is thrown somewhere along the line we now don't lose the information of which migrations were executed
  • Update the logging configuration loader to use logging.json as the system-specific overrides instead of logging.user.json
  • Update the logging migration. It no longer needs to create logging.user.json, which is not used any more. Instead, it checks if logging.json is unmodified by the user and, if yes, moves it to logging.old.json.
  • Use Guids instead of a string names to uniquely identify migrations. This has the side effect that migrations will be re-run for anyone on nightly because the unique identifiers have changed, but the single existing migration is safe to run multiple times anyways (it just sets encoding.EnableThrottling to false)

Jellyfin.Server/Migrations/MigrationOptions.cs Outdated Show resolved Hide resolved
@mark-monteiro
Copy link
Member Author

Pushed one more change to store the migration name alongside the id in the config file's list of applied migrations. Only the Guid is used to uniquely identify the migration in the runner, but the name is added to assist with development/debugging

@joshuaboniface joshuaboniface merged commit d8d3767 into jellyfin:master Mar 8, 2020
Release 10.5.0 automation moved this from In progress to Done Mar 8, 2020
@mark-monteiro mark-monteiro deleted the logging-migration branch March 8, 2020 21:33
@Bond-009
Copy link
Member

Bond-009 commented Mar 9, 2020

Why did this functionality get added to Jellyfin.Server? imo it should be added to Emby.Server.Implementations.
The default config shouldn’t be in the same dir as the user config

@mark-monteiro
Copy link
Member Author

Why did this functionality get added to Jellyfin.Server? imo it should be added to Emby.Server.Implementations.

What's the difference between these two projects? I'm not super familiar with the project layout yet.

The default config shouldn’t be in the same dir as the user config

Why not? Where would be an appropriate place to put it?

@Bond-009
Copy link
Member

Bond-009 commented Mar 10, 2020

Jellyfin.Server is for platform specific code, things that require netcoreapp and setup. Emby.Server.Implementations is everything else we need and can use in xamarin for example without modifications.

Following the xdg spec:

Default configuration files should be installed to $sysconfdir/xdg/subdir/filename with $sysconfdir defaulting to /etc.

Ref: https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html

Afaik it’s fine on windows as is

@mark-monteiro
Copy link
Member Author

mark-monteiro commented Mar 10, 2020

Thanks for the clarification, I opened #2564 to track updating the location of the default file.

EDIT: And I've also adding moving the migration running logic to Emby.Server.Implementations to #2538

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Support changes to logging configuration defaults
4 participants