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

Search user configuration directory for the configuration file #576

Closed
wants to merge 1 commit into from
Closed

Search user configuration directory for the configuration file #576

wants to merge 1 commit into from

Conversation

evanpurkhiser
Copy link

This is a incomplete pull request. But I'd like to get some comments on this. I've been toying around with using supervisor to manage daemons in my user session for a light weight desktop (window manager, hoktkey listener, etc). One downside is having to specify the configuration file on the command line, since supervisor doesn't search in the users path.

Again, this is just a springboard PR for comments on the idea. I can add tests and make any changes based on comments, but the idea is there!

@mnaberez
Copy link
Member

Related: #528, #414

In the past, the search paths have caused a lot of confusion. Issues relating to the config file search often come up on the tracker. I think there are three issues:

  1. The size of search list is already large and confusing. It is difficult to explain how config file search works to new users.
  2. The existence of the relative paths in the search list. The relative ones seem especially problematic for users on the issue tracker. I didn't add those, but I think they were done to support some use case involving virtualenv and/or buildout. Users who are not using those systems are surprised when Supervisor behaves a different way when run from certain directories.
  3. Some distributions have modified the search list, e.g. for years Debian and Ubuntu have modified Supervisor to search for a different location. This is confusing to users because Supervisor doesn't behave on their system as our documentation describes. This is issue Added standard Debian location for Supervisor configuration file #414.

I am unlikely to merge this particular change (#576) because it adds another location to the search, which is probably too long already. It also changes the name of the config file from supervisord.conf to config.conf which is unlike all the other paths we search. I understand that change was done to support a use case, but the use case seems non-typical for Supervisor.

While I do not want to grow the search list, I have been thinking about merging #414 for a long time because some distributions have already patched Supervisor, and those have been shipping for years. Many users seem to install Supervisor with a distribution package and are surprised loading the config file doesn't work like our documentation describes. For that path, I think it might make sense, even though it grows the search path list.

It may be too late to remove paths from the search list. At this point, I am just trying not to make the situation worse. If we could do it again, I would be tempted to have it search /etc/supervisord.conf and /etc/supervisor/supervisord.conf and nothing else, and advise all other users to use -c with an explicit config. It would mean that I'd have to start using -c on my systems too. This is based on my experience on the issue tracker and mailing list.

I would welcome more thoughts and experiences with the search list.

cc @msabramo

@WhyNotHugo
Copy link

I understand the interest of not-growing the list and I concur that the simpler the config search is, the more usable the application becomes.

However, there is an issue with the current list if a non-root user intends to use supervisord: there's no default configuration file:

  • /etc/ is unwriteable for non-root.
  • $CWD varies. If I run supervisorctl on any dir, the default will vary as well. That's not even a sane default for something like this, and I believe that actually removing it would actually reduce confusion.

~/.config (or the xdg-basedir spec) (and of course keeping /etc/) are actually sane defaults for most users: root will most likely use /etc and any other user ~/.config.

Those that rely on $CWD can probably just -c, IMHO. I guess this will probably trigger some debate, but searching for the "default config" in $CWD is rather unorthodox and unexpected too.

@mnaberez
Copy link
Member

However, there is an issue with the current list if a non-root user intends to use supervisord:
there's no default configuration file

Is that a problem? This pull request and your comment seem to indicate it might be, but this is the first time I can remember that the issue has been raised. I've been running my Supervisor instances as non-root for years using -c.

@WhyNotHugo
Copy link

Is that a problem? This pull request and your comment seem to indicate it might be, but this is the first time I can remember that the issue has been raised. I've been running my Supervisor instances as non-root for years using -c.

Well, it does reduce comfort a bit. Having to specify -c on every single usage is a bit of a pain. I mean, this can be up to many dozens of times a day. Imagine if every app out there required specifying the configuration file location: it'd be a pain. There's no reason not to have a well-defined default (IMHO, $CWD/filename is not well-defined).

Another option could, of course, be an environment variable, since that can include either relative or absolute paths, and that satisfies more scenarios.

@mnaberez
Copy link
Member

Well, it does reduce comfort a bit. Having to specify -c on every single usage is a bit of a pain.

I can understand that. I may not be feeling it because I usually interact with it through automation.

Imagine if every app out there required specifying the configuration file location: it'd be a pain.

I'd like to keep this focused on Supervisor. It does have a bunch of defaults, just not this particular one. Since Supervisor has been around for a long time, and this seems to be the first time this specific issue has been raised, it may not affect that many people.

@WhyNotHugo
Copy link

I'd like to keep this focused on Supervisor. It does have a bunch of defaults, just not this particular one.

There's no real single default if it's not run as root. /etc is not writeable for normal users, and a relative path doesn't count (it changes constantly, making it something one can't depend on. The relative path isn't very sane either: it means a user needs to cd into a specific directory before every interaction with supervisorctl, which is basically the same as typing the configuration file path as an argument itself.

I may not be feeling it because I usually interact with it through automation.

Since you don't actually use supervisorctl, then you can't really understand the hassle of specifying a configuration file upon each usage.


In order to avoid increasing the list indefinitely, I'd like to propose using an environment variable (maybe SUPERVISOR_CONFIG. This means:

  • No new search path unless the user manually and explicitly set this variable. So no behavioural change for anybody currently using supervisor.
  • Can be used to conform to any usage and path preference, no matter how unique.
  • Can be used so that [non-root] users have a default and writeable configuration file.
  • Can actually be used as an excuse to remove paths in future, should interest for simplification arise (most likely in a major release, due to being a backwards incompatible change). I'm suggesting this merely because it's been stated a few times that the search list is too long, and there may be desire to actually shorten it.

Would this be acceptable if a PR was pushed?

@mnaberez mnaberez closed this Apr 30, 2015
@Supervisor Supervisor locked and limited conversation to collaborators Apr 30, 2015
@mnaberez
Copy link
Member

Since you don't actually use supervisorctl, then you can't really understand the hassle of specifying a configuration file upon each usage.

OK, this is a little out of control now. Since this is just mud-slinging now and nothing productive is coming out of it, I'm going to lock this topic. FTR, I use supervisorctl every day, have been maintaining it for five years, and worked on it for almost another five before that. I have typed the -c option many, many times.

In order to avoid increasing the list indefinitely, I'd like to propose using an environment variable (maybe SUPERVISOR_CONFIG.

It's complicated enough without adding environment variables to the search path in my opinion. Since nobody else has chimed in the two months that this discussion has been open, maybe this just isn't that important to most people.

@Supervisor Supervisor unlocked this conversation Jun 6, 2015
@mnaberez
Copy link
Member

Several people have now requested letting the config filename be set by a SUPERVISOR_CONFIG environment variable so I've come around on it. Issue #742 is open for it.

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

Successfully merging this pull request may close these issues.

3 participants