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

gollum service: init #29476

Merged
merged 1 commit into from Sep 18, 2017
Merged

gollum service: init #29476

merged 1 commit into from Sep 18, 2017

Conversation

erictapen
Copy link
Member

Motivation for this change

This introduces the already packaged gollum package as a nixos module. Example config:

  services.gollum = {
    enable = true;
    port = 12486;
  };

I mainly copied code from the gitit module (which I used before gollum). As gitit is commented out for quite some time, it may be that I used some outdated coding conventions. Also I'm not that familiar with systemd services. Feedback is welcome.

As I use gollum personally, I will continue to add features to it and will do maintenance.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • 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/)
  • Fits CONTRIBUTING.md.

@mention-bot
Copy link

@erictapen, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @joachifm and @offlinehacker to be potential reviewers.

joachifm
joachifm reviewed Sep 17, 2017
View reviewed changes
nixos/modules/services/misc/gollum.nix Outdated
mkdir -p ${cfg.stateDir}
git init ${cfg.stateDir}
chmod 755 ${cfg.stateDir}
chown -R gollum:gollum ${cfg.stateDir}
Copy link
Contributor

Choose a reason for hiding this comment

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

Fixing ownership at startup seems to make allocating a static uid redundant. If a static uid must be used, this is at least a waste of time.

Copy link
Member Author

@erictapen erictapen Sep 17, 2017 •
edited

Choose a reason for hiding this comment

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

Thanks! This part was meant as a temporarily solution I forgot to fix. I'd like to let users the choice of renaming the gollum user. This means, that line 78 is supposed to look like this:

chown -R ${cfg.user}:${cfg.group} ${cfg.stateDir}

If you think, that with this solution I would'nt need a static UID/GID, I will remove them.

Copy link
Contributor

Choose a reason for hiding this comment

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

With fixing the ownership, I expect a static uid is unnecessary. If updating the ownership is very expensive, however, perhaps stick with only static uid. Then again, I don't really know the software so I'll leave that to your judgement.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now I think I got what you said. I removed the static UID/GID. That generic userName/groupName feature i mentioned will be added later, as I don't see immediate need for it.

joachifm
joachifm reviewed Sep 17, 2017
View reviewed changes
nixos/modules/services/misc/gollum.nix Outdated
description = "Enable the Gollum service.";
};

package = mkOption {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a use case for using a package other than the default?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, I just saw this pattern in other packages. Will remove it.

joachifm
joachifm reviewed Sep 17, 2017
View reviewed changes
nixos/modules/services/misc/gollum.nix Outdated
};

extraConfig = mkOption {
type = types.str;
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using types.lines, it has more sensible merge semantics for an option like this.

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense. Updated the PR.

@erictapen erictapen force-pushed the gollum-service-init branch 4 times, most recently from 309d83d to 8ed4a8c Compare September 17, 2017 11:01
joachifm
joachifm reviewed Sep 17, 2017
View reviewed changes
nixos/modules/services/misc/gollum.nix Outdated
createHome = false;
};

users.groups.gollum.name = config.users.users.gollum.name;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can also do simply

let # for syntax hl, ignore
users.groups.gollum = { };

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah nice. Added this (untested as it is trivial).

@fpletz fpletz merged commit b5a5d0b into NixOS:master Sep 18, 2017
@erictapen
Copy link
Member Author

Yeah, thanks alot!

@erictapen erictapen deleted the gollum-service-init branch September 18, 2017 10:16
@fpletz fpletz added this to the 17.09 milestone Sep 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joachifm joachifm joachifm left review comments

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (new)
Projects
None yet
Milestone
17.09
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@erictapen @mention-bot @joachifm @fpletz