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

Check Config File Existence #4054

Merged
merged 4 commits into from Dec 13, 2018
Merged

Check Config File Existence #4054

merged 4 commits into from Dec 13, 2018

Conversation

Tensho
Copy link
Contributor

@Tensho Tensho commented Dec 11, 2018 •
edited

Ispired by #4047, Fixes #3735.

Parse sidekiq.yml[.erb] only in case:

  • :config_file is not present as CLI argument
  • :require is a directory (Rails application)
  • Expanded path to config/sidekiq.yml[.erb] against :require exists

Example when default config file is considered:

$ tree haiku
haiku
└─ config
    └─ sidekiq.yml
$ sidekiq -v -r ./haiku
...
:require => "./haiku"
:config_file => "/Users/tensho/Projects/sidekiq/haiku/config/sidekiq.yml"
...

@Tensho Tensho mentioned this pull request Dec 12, 2018
Closed
mperham
mperham reviewed Dec 12, 2018
View reviewed changes
test/test_cli.rb Outdated Show resolved Hide resolved
@mperham mperham merged commit 1d83555 into sidekiq:master Dec 13, 2018
@mperham
Copy link
Collaborator

mperham commented Dec 13, 2018

Thanks!

@Tensho Tensho deleted the check-config-file-existence branch December 13, 2018 16:41
Tensho added a commit to Tensho/sidekiq that referenced this pull request Dec 22, 2018
@Tensho
Check Config File Existence (sidekiq#4054)
794a465 
* Check config file existence

* Eager config file check

* Parse expanded path to default sidekiq.yml config file in Rails app

* Cleanup
Tensho added a commit to Tensho/sidekiq that referenced this pull request Dec 28, 2018
@Tensho
Check Config File Existence (sidekiq#4054)
8cad1fa 
* Check config file existence

* Eager config file check

* Parse expanded path to default sidekiq.yml config file in Rails app

* Cleanup
mperham pushed a commit that referenced this pull request Dec 28, 2018
@Tensho @mperham
JSON Logger Formatter (#4050)
3845832 
* Check Config File Existence (#4054)

* Check config file existence

* Eager config file check

* Parse expanded path to default sidekiq.yml config file in Rails app

* Cleanup

* Add minitest-around

* Extract context from formatter

* Add JSON logger formatter

* Adjust job logger to handle elapsed time within context

* Add tid test

* Rename processor logger

* Enforce global state reset in logging tests

* Add warning about upcoming refactoring to Sidekiq::Logging

* Replace around hook with explicit stub inside test

It's implemented with fibers, which means Thread.current returns different values in JRuby.

* Fix typo

* Concise JSON formatter keys

* Add logger_formatter option

* Shift context from array of strings to hash

Allows more flexibly format context in the different formatters.

* Adjust warning message regarding context type change

* Add "Formatter" suffix to classes

* Fix CLI specs

* Replace Sidekiq::Logging with Sidekiq::Logger

* Namespace logger formatters

* Remove rails 4 appraisal
@navied
Copy link

navied commented Jan 7, 2019

This is a bit of breaking change, the previous behavior is to check the default config directories on startup now a config flag is required. Might want to make a note of this in the change log. @mperham

@mperham
Copy link
Collaborator

mperham commented Jan 7, 2019

@navied Could you be more specific? Sidekiq should always pick up the default config file if none is specified. If it does not, that's a bug.

@navied
Copy link

navied commented Jan 7, 2019

Prior to 5.2.4, in a non-rails sidekiq setup the default config folder of config/sidekiq.yml was picked up. But the default config file is still picked up with a rails w/ sidekiq setup.

@mperham
Copy link
Collaborator

mperham commented Jan 7, 2019 •
edited

Thanks, @Tensho do you have time to look into this?

navied
navied reviewed Jan 7, 2019
View reviewed changes
Copy link

@navied navied left a comment

Choose a reason for hiding this comment

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

No problem, I pointed out the part of the code I think might be the issue.

lib/sidekiq/cli.rb

%w[config/sidekiq.yml config/sidekiq.yml.erb].each do |filename|
Copy link

Choose a reason for hiding this comment

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

Missing code that is potentially the issue.

@mperham mperham mentioned this pull request Jan 8, 2019
Closed
@Tensho
Copy link
Contributor Author

Tensho commented Jan 8, 2019 •
edited

I wonder sidekiq supports default config file in config folder aside of the Rails applications. However, the documentation provides the pretty clear statement, without Rails connotation:

The Sidekiq configuration file is a YAML file that Sidekiq server uses to configure itself, by default located at config/sidekiq.yml.

Sorry for that oversight 🙇 I'll make an appropriate change.

@Tensho
Copy link
Contributor Author

Tensho commented Jan 8, 2019

Adjusted in #4077

@patbl patbl mentioned this pull request Oct 26, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mperham mperham mperham left review comments

@navied navied navied left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Tensho @mperham @navied