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

replace deprecated usage of PermissionsStartOnly (part 2) #56265

Merged
merged 34 commits into from Jun 25, 2019

Conversation

@aanderse
Copy link
Contributor

commented Feb 23, 2019

Motivation for this change

#53852

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@flokli

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2019

@aanderse something's looking weird here - did you push some on the branch than this PR?

@aanderse aanderse force-pushed the aanderse:permissions-start-only branch from dddbff7 to 862c38a Feb 24, 2019

@aanderse

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2019

@aanderse something's looking weird here - did you push some on the branch than this PR?

@flokli oops! pardon my git stupidity 😄

@flokli

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2019

No worries! Looks better now :-)

How do you want to proceed with this? Gathering feedback from module maintainers? Running VM tests?

@aanderse

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2019

I'm guessing quite a few of these modules don't have tests.

In rare situations there could be implications in changing from a mkdir+chmod in the preStart script (which runs every time the service starts) to tmpfiles (which doesn't run every time the service starts, from what I read).

Given that I don't know or use many of these modules I'd say run a test when it exists as a first pass, but also ping maintainers, then we can cherry pick individual module commits one by one. Given the timeline I think that should work out.

For now I plan to hammer away at the brainless modules which are low hanging fruit, and then progressively work my way up to modules which require more thought and understanding of the program/service. That being said, if anyone wants to start pinging maintainers so individual commits can be cherry picked feel free.

@aanderse

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

Update as of 2019-04-13: Thanks to everyone for their review and feedback. The first batch of changes which have either been reviewed or have a passing NixOS test are now going to be merged into master and then removed from this PR.


Quick Summary: PermissionsStartOnly is deprecated and a module that you are listed as a maintainer for uses it. I have patched the module you maintain to not use PermissionsStartOnly anymore, but I would like a module maintainer (ie. you) to review the changes and sign off on it.

More details: #53852

Please review the list below for your name and review the changes to your module(s). I haven't ran tests yet, so if you want to run a test and confirm everything is working, that all the changes makes sense, and there won't be any problems from my changes, that would be appreciated.

If you have the ability to merge please go ahead and cherry pick the commit relevant to your module(s) if everything checks out and you're happy with the changes and then check off your module. If you do not have commit access please leave a comment stating that you accept the changes and are OK for them to be merged as is.

If there are problems with the changes it would be great if you could provide a fix, or at least explain what the issue(s) are.

Thank you for your time and effort.

Note: If a module is checked that means no further action is required (because the module has already been merged, deprecated, taken care of by someone else, etc...)

reviewed

not reviewed, but covered by tests

not reviewed, not covered

  • alerta
  • apache-kafka @ragnard does the commit listed look good to you?
  • aria2 @jgeerds does the commit listed look good to you?
  • autossh @pSub does the commit listed look good to you?
  • boinc
  • charybdis @Lassulus @fpletz does the commit listed look good to you?
  • confluence
  • couchpotato @fadenb does the commit listed look good to you?
  • crowd
  • dspam @abbradar does the commit listed look good to you?
  • firebird @MarcWeber does the commit listed look good to you?
  • foundationdb @thoughtpolice does the commit listed look good to you?
  • frab @fpletz @markuskowa this is marked as broken, should it be abandoned?
  • graylog @fadenb does the commit listed look good to you?
  • hbase
  • heartbeat @ehmry @lethalman does the commit listed look good to you?
  • hydron
  • jira
  • kapacitor @offlinehacker @ehmry @lethalman does the commit listed look good to you?
  • minidlna
  • mopidy @rickynils @fpletz does the commit listed look good to you?
  • murmur @jgeerds @wkennington does the commit listed look good to you?
  • nagios @thoughtpolice does the commit listed look good to you?
  • netadata @lethalman does the commit listed look good to you?
  • octoprint @abbradar does the commit listed look good to you?
  • opendkim @abbradar does the commit listed look good to you?
  • quassel @Phreedom @ttuegel does the commit listed look good to you?
  • riemann-dash @rickynils does the commit listed look good to you?
  • riemann-tools
  • scollector @offlinehacker @ehmry @lethalman does the commit listed look good to you?
  • slimserver @phile314 does the commit listed look good to you?
  • squid @fpletz does the commit listed look good to you?
  • teamspeak3
  • unifi

module to be deprecated

  • emby @fadenb does the commit listed look good to you?
  • rmilter @avnik @fpletz does the commit listed look good to you?
  • winstone

modules which I haven't written code for (yet?)

  • acme
  • aerospike
  • bosun
  • consul
  • datadog-agent
  • dkimproxy-out
  • ejabberd
  • elasticsearch
  • errbot
  • exhibitor
  • firefox
  • gale
  • gammu-smsd
  • geoip-updater
  • gitlab
  • graphite
  • infinoted
  • hydra
  • lirc
  • journalwatch
  • kippo
  • kubernetes
  • mattermost
  • matomo
  • matrix-synapse
  • meguca
  • mongodb
  • mwlib
  • neo4j
  • opentsdb
  • plex
  • postgresql
  • postsrsd
  • rethinkdb
  • riak
  • riak-cs
  • spiped
  • squeezelite
  • sslh
  • tarsnap
  • taskserver
  • tomcat
  • tt-rss
  • varnish
  • xrdp
@andrew-d

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

For syncthing, LGTM - the module doesn't appear to use any ExecStart/ExecStop/ExecReloads, so removing PermissionsStartOnly shouldn't have any effect.

@mdaiter

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

@aanderse looks good!

@bachp

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

For minio LGTM

@markuskowa

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

Looks good. The tests for postgresql-backup and munge (it's the slurm test) are passing.

@aanderse aanderse force-pushed the aanderse:permissions-start-only branch from 6f51131 to de6e5ea May 26, 2019

@Ma27

This comment has been minimized.

Copy link
Member

commented May 26, 2019

Regarding the Atlassian modules: I guess that @globin and @fpletz may be qualified here.

@@ -70,25 +70,25 @@ in {

config = mkIf cfg.enable {

systemd.tmpfiles.rules = [
"d '${cfg.dataDir}' - mopidy mopidy - -"

This comment has been minimized.

Copy link
@Mic92

Mic92 May 31, 2019

Contributor

Is the ownership applied recursive by the way?

This comment has been minimized.

Copy link
@aanderse

aanderse May 31, 2019

Author Contributor

No. I only added the set recursive "Z" option when someone explicitly requested/thought was required.

@Mic92

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

To move this forward, I would suggest to split this pr into smaller ones.
I would prefer to review 10 services at the time instead of 34.

@aanderse

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

@Mic92 This is split into smaller. Normally I agree that too many changes are hard to digest, but in this case I thought everything is similar so I would keep in fewer PRs with 1 commit per change unless explicitly asked. Does anyone else request more PRs to split this up?

@andir

This comment has been minimized.

Copy link
Member

commented Jun 16, 2019

@aanderse gollum looks fine. I tried to modify a few pages and didn't see any issues.

@grahamc

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

Looks good, and looks ilke a ton of great work. Thank you a lot! Let's let this bake on unstable a bit.

@grahamc grahamc merged commit 38c28ef into NixOS:master Jun 25, 2019

12 checks passed

Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details

@aanderse aanderse deleted the aanderse:permissions-start-only branch Jul 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.