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

fix: fetchmail environment variables #3901

Merged
merged 2 commits into from Feb 21, 2024

Conversation

casperklein
Copy link
Member

@casperklein casperklein commented Feb 20, 2024

Description

Executing supervisor restart fetchmail causes:

Feb 20 17:12:45 mail fetchmail[2543]: /root/.netrc: cannot open file for reading: Permission denied

Fetchmail checks the wrong location to see if .netrc exists. That's because $HOME is set to /root. fetchmail does not have permission to access /root.

http://supervisord.org/subprocess.html#subprocess-environment

No shell is executed by supervisord when it runs a subprocess, so environment variables such as USER, PATH, HOME, SHELL, LOGNAME, etc. are not changed from their defaults or otherwise reassigned. This is particularly important to note when you are running a program from a supervisord run as root with a user= stanza in the configuration

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (non-breaking change that does improve existing functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (README.md or the documentation under docs/)
  • If necessary, I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have added information about changes made in this PR to CHANGELOG.md

@casperklein casperklein self-assigned this Feb 20, 2024
@casperklein casperklein added the kind/bug/fix A fix (PR) for a confirmed bug label Feb 20, 2024
@casperklein casperklein added this to the v14.0.0 milestone Feb 20, 2024
@casperklein casperklein marked this pull request as ready for review February 20, 2024 21:50
@casperklein
Copy link
Member Author

@georglauterbach @polarathene Not related to this PR and just cosmetic: What is your opinion about renaming target/supervisor/conf.d/supervisor-app.conf to something like DMS.conf or similar?

@georglauterbach
Copy link
Member

@georglauterbach @polarathene Not related to this PR and just cosmetic: What is your opinion about renaming target/supervisor/conf.d/supervisor-app.conf to something like DMS.conf or similar?

Not an issue if you ask; we should classify it as breaking, though.

@polarathene
Copy link
Member

What is your opinion about renaming target/supervisor/conf.d/supervisor-app.conf to something like DMS.conf or similar?

I don't see any value in doing so (especially as uppercase). I'd rather a motivation for it, as we do have a separate config for sasl login mechanisms too, you could probably split it into separate service groups if that makes more sense? 🤷‍♂️

@casperklein
Copy link
Member Author

I think "supervisor-app" is completely generic. The "app" (all services combined) we are using is called docker-mailserver (DMS). So it makes more sense to me, to name the config accordingly.

@casperklein casperklein merged commit e232e43 into docker-mailserver:master Feb 21, 2024
7 checks passed
@casperklein casperklein deleted the fix-fetchmail branch February 21, 2024 10:19
@georglauterbach
Copy link
Member

georglauterbach commented Feb 21, 2024

Having service groups is something I'd support as well. Maybe for anti-spam, then dovecot-specific, and then dms.conf for the rest?

@casperklein
Copy link
Member Author

Having it all in one place, like now, is fine for me. The file isn't that big or confusing that would justify a splitting IMO.
It is really just the name supervisor-app, that triggers me for some time now 😆 If you both want to keep that name, that's fine 👍

@georglauterbach
Copy link
Member

Nah, go ahead and change it to dms.conf, I like that one better too.

@polarathene
Copy link
Member

The file isn't that big or confusing that would justify a splitting IMO.

I was thinking more in terms of feature isolation. I probably won't get around to it, but thought about exploring a file structure that groups scripts related to features/services together, just gets a bit messy when integration is involved. Probably not worth it.

It is really just the name supervisor-app, that triggers me for some time now 😆 If you both want to keep that name, that's fine 👍

I don't mind either way. dms-services.conf might be more appropriate than just dms.conf if you do go forward with such a change. Might be best to schedule it for v15, as v14 breaking changes is already quite a large list in changelog 😅

@casperklein
Copy link
Member Author

dms-services.conf sounds perfect 👍

Might be best to schedule it for v15, as v14 breaking changes is already quite a large list in changelog

One more tiny change makes no big difference IMO. But I don't care if this is done in v14 or 15.

@georglauterbach
Copy link
Member

I agree that this can easily go into v14.

@polarathene
Copy link
Member

Fair enough 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug/fix A fix (PR) for a confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants