nixos/gitolite: customize .gitolite.rc declaratively #29295

pvgoran · 2017-09-13T02:50:29Z

Add the extraGitoliteRc option to customize the .gitolite.rc configuration file declaratively.

Resolves #29249.

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 · 2017-09-13T02:50:30Z

@pvgoran, thanks for your PR! By analyzing the history of the files in this pull request, we identified @thoughtpolice, @abbradar and @bjornfor to be potential reviewers.

pvgoran · 2017-09-13T03:00:56Z

I'm somewhat confused about the desired commit topic format. https://github.com/NixOS/nixpkgs/blob/master/.github/CONTRIBUTING.md says I should use nixos/gitolite: <description>, whereas https://nixos.org/nixpkgs/manual says I should use gitolite service: <description>. Or maybe the commit topic should be different from the pull request topic?

I'd love to see some clarification to this. :)

bjornfor · 2017-09-13T05:59:45Z

I suggested/added the "nixos/<thing>" to CONTRIBUTING.md some weeks ago. I didn't know that information was also in the manual :-/ The manual should be updated.

bjornfor · 2017-09-13T12:15:37Z

I updated the Nixpkgs manual on master (56a047c) and release-17.09 (dcb66ca).

pvgoran · 2017-09-13T12:23:16Z

Thank you!

bjornfor · 2017-09-13T15:36:23Z

nixos/modules/services/misc/gitolite.nix

@@ -49,6 +62,17 @@ in
        '';
      };

+      extraGitoliteRc = mkOption {
+        type = types.str;


If it makes sense that extraGitoliteRc can be set in multiple modules, better change from types.str to types.lines so that multiple definitision can be merged.

Well, I think it's possible for it to make sense. If anything, it can make implementation of the upcoming enableGitAnnex option a bit shorter. However, I can't think why end users would want to separate services.gitolite configuration into multiple modules.

Taking this in consideration, what would you advise? Should I change the type?

If it works with types.lines I think we should do it. You never know when someone would like to modularize their configuration. (For instance, I have my samba smb.conf defined in two modules.)

Ok, I'll make the change.

bjornfor · 2017-09-13T16:56:00Z

Cool! I didn't know that it was possible to append things to .gitolite.rc like that. So far I've been maintaining mine manually.

I know very little perl. Does this method allow customizing/overriding everything in .gitolite.rc? As in, here are no drawbacks by this "append" style of manipulating the config file?

Having multiple

# per perl rules, this should be the last line in such a file:
1;

sections in the file is OK etc.?

bjornfor · 2017-09-13T16:58:23Z

nixos/modules/services/misc/gitolite.nix

@@ -71,7 +95,6 @@ in
    systemd.services."gitolite-init" = {
      description = "Gitolite initialization";
      wantedBy    = [ "multi-user.target" ];
-      unitConfig.RequiresMountsFor = cfg.dataDir;


Removing this line looks like a bad conflict resolution.

Right. I didn't even notice it. :(

bjornfor · 2017-09-13T17:01:57Z

nixos/modules/services/misc/gitolite.nix

@@ -7,6 +7,19 @@ let
  # Use writeTextDir to not leak Nix store hash into file name
  pubkeyFile = (pkgs.writeTextDir "gitolite-admin.pub" cfg.adminPubkey) + "/gitolite-admin.pub";
  hooks = lib.concatMapStrings (hook: "${hook} ") cfg.commonHooks;
+  orNull = cond: val: if cond then val else null;
+  rcAppendFile =
+    orNull (cfg.extraGitoliteRc != "") (pkgs.writeText "gitolite-rc-append" rcAppendText);


IMHO, I think it'd be easier to read if using plain "if/then/else" instead of having the "orNull" function.

Well, it would make the line too long, or require splitting the if-then-else construct. I'll see if it can be rewritten elegantly.

bjornfor · 2017-09-13T17:03:25Z

nixos/modules/services/misc/gitolite.nix

@@ -81,6 +104,12 @@ in
      script = ''
        cd ${cfg.dataDir}
        mkdir -p .gitolite/logs
+        >.gitolite.rc gitolite print-default-rc


I didn't know IO redirection could be done at the start of the line.

It seems to work in the middle of the line as well.

Do you mind changing the IO redirect to the end of the line? My guess is that having it at end-of-line is more common and people reading the code won't have to stop and think about what the code does if it looks more familiar. (At least that's how I feel.)

Well, placing the redirects at the beginning creates prominent similarity between the commands that write to the file, therefore visually "grouping" them. This was my motivation for placing redirects like this. However, a predictable, well-known way of doing things is probably more valuable.

bjornfor · 2017-09-13T17:05:31Z

So with this PR, .gitolite.rc will be fully declarative. But still the file will be writeable, only to be overwritten on every service start. Could we instead make .gitolite.rc a symlink to /nix/store/...-gitolite.rc?

pvgoran · 2017-09-13T17:20:42Z

Does this method allow customizing/overriding everything in .gitolite.rc?

It should be possible to customize everything, but it would require some Perl knowledge. (For example, removing items from a list is a bit more complex than adding them.)

As in, here are no drawbacks by this "append" style of manipulating the config file?

Well, if the structure of the configuration defined in the (default) .gitolite.rc changes, then the overrides may stop working. Other than that, I think it's safe.

Having multiple (...) sections in the file is OK etc.?

Yes. They are just expressions; the last expression in the file determines its return value, which should be non-zero.

But still the file will be writeable, only to be overwritten on every service start. Could we instead make .gitolite.rc a symlink to /nix/store/...-gitolite.rc?

I don't know a proper way of implementing it. Can we run gitolite print-default-rc during the configuration build, and save the result in the store? I have a feeling this would be much more difficult than my current implementation.

bjornfor · 2017-09-13T18:46:01Z

Sounds good!

You can make a gitolite.rc file in the Nix store like this:

with import <nixpkgs> {};

runCommand "gitolite.rc" {}
  ''
    export HOME=gitolite-home
    mkdir -p gitolite-home/.gitolite/logs
    ${gitolite}/bin/gitolite print-default-rc > "$out"

    cat << EOF >> "$out"

    # extraGitoliteRc stuff
    EOF
  ''

If gitolite works with .gitolite.rc being a symlink to this nix store file, I think that's preferable. It gives a much stronger signal that the file is managed declaratively.

bjornfor · 2017-09-13T19:08:23Z

Perhaps we should think about the upgrade path too. For users already having a .gitolite.rc (like me), it's not so nice to just blow it away.

How about this. Assuming the symlink to nix store thing works, we can have the gitolite service check the type of the .gitolite.rc file. If it's a plain file, abort the service with message like

"""Since NixOS 17.09, .gitolite.rc is managed declaratively and customizations must be done with the services.gitolite.extraGitoliteRc option. You currently have a .gitolite.rc file that is not managed by NixOS. Please take any customizations you may have in .gitolite.rc and move them to the new extraGitoliteRc option. Then remove ${datadir}/.gitolite.rc to allow NixOS to manage the file for you."""

If the .gitolite.rc file is a symlink, we check if it points to the /nix/store. If it does, find then we replace it with a symlink to the new file. If it's a symlink but not pointing to /nix/store, abort with the same message as above. (Or print a warning instead of aborting?)

Thoughts?

bjornfor

I'd like to see attempt to use .gitolite.rc from nix store to prevent accidental user changes being overwritten. Also, consider upgrade path for users already having an imperatively configured .gitolite.rc file. (Possible solution outlined in comment.)

pvgoran · 2017-09-13T20:07:38Z

You can make a gitolite.rc file in the Nix store like this:

So, runCommand generates the filename for the script output, passes this filename as $out (shell variable) to the script, and then places the resulting file to the Nix store and finally returns the path of the saved file? Is this correct?

Also, what makes it possible to create gitolite-home in your example and not care about it afterwards? Is this script run in some kind of temporary directory that gets wiped once it's completed?

If gitolite works with .gitolite.rc being a symlink to this nix store file, I think that's preferable. It gives a much stronger signal that the file is managed declaratively.

I assume it does. At least, gitolite's documentation mentions this as a possible approach (see below).

Thoughts?

This looks good to me. I don't think the script should abort if a "legacy" .gitolite.rc is found; a warning message would be better. Speaking of which, does systemd have some kind of special commands for the services to issue warnings?

Also, users may have valid reasons not to manage .gitolite.rc via NixOS configuration. For example, they may symlink it to a file inside the gitolite-admin repository, to manage the configuration via git. gitolite documentation describes this setup, although it isn't recommended. For such users, I would add an additional option, something like manageGitoliteRc with a default of true, which would give a choice to leave the file alone and not issue warnings.

bjornfor · 2017-09-13T20:29:07Z

So, runCommand generates the filename [...]

Correct.

Also, what makes it possible to create gitolite-home in your example and not care about it afterwards?

It seems gitolite just needs a place to store log files. I would be surprised if the files will be needed afterwards. I found out this by trial and error. I didn't find flag to disable logging to file.

Is this script run in some kind of temporary directory that gets wiped once it's completed?

Correct. That directory will be created in the temporary build directory of the derivation (typically /tmp/nix-build-* something, IIRC).

does systemd have some kind of special commands for the services to issue warnings?

You can prefix messages with a number in angle brackets, indicating severity level. echo "<3>this is an error message". Also, I think I've seen a service making "systemctl status theservice" show a dedicated "Status: " field, but I don't know how that's implemented. I think echoing a message with severity level is fine.

Also, users may have valid reasons not to manage .gitolite.rc via NixOS configuration. For example, they may symlink it to a file inside the gitolite-admin repository, to manage the configuration via git. gitolite documentation describes this setup, although it isn't recommended. For such users, I would add an additional option, something like manageGitoliteRc with a default of true, which would give a choice to leave the file alone and not issue warnings.

Good idea.

pvgoran · 2017-09-14T09:08:32Z

What is the proper procedure for changing the pull request? Should I just add another commit to the branch, or replace/amend the commit and force-push the branch?

bjornfor · 2017-09-14T13:04:43Z

Amend and force push.

pvgoran · 2017-09-14T17:17:00Z

I think I addressed everything with the new commit. Here are (somewhat long) list of concerns I have with relation to this code:

I'm not sure that additional explanations in description of extraGitoliteRc are necessary.
How do I properly add an assert to the module definition, to ensure that module options are compatible? I wanted to add assert (cfg.manageGitoliteRc || cfg.extraGitoliteRc == ""); to the definition of config after mkIf cfg.enable, but it resulted in infinite recursion. Currently this assert is in the definition of systemd.services."gitolite-init", which looks quite unnatural (it isn't about the gitolite-init service, it's about the entire module), but at least it works. Is there a better way? Is it possible to attach a error message to the assert?
I'm confused by the way systemd's journal is shown. In my terminal (110 characters width), when the gitolite-init service issues the warning that I added, almost half of the terminal width is occupied with auxilary information, so I only see 50 characters for the actual message:

Sep 14 19:49:06 nixos gitolite-init-start[14074]: Alternatively, you can set services.gitolite.manageGitoliteR

I can use the right arrow key to see the remainder of overlong lines, but it's very hard to read the log if I have to scroll the screen left and right for each line I read.

For now, I limited the width of message lines to ~50 characters so that they would fit in my terminal, but this is obviously non-optimal: (1) this way, the message takes 11 lines which seems too much, (2) users will have different terminal width, for some 50-character lines will be too long, for some they will be too short. Unless I'm missing something and there is a better way to access systemd logs, it looks like I can't format the warning message in a way that would be convenient for most users. Do you have any recommendation of what to do here?

bjornfor · 2017-09-14T19:15:12Z

I'm not sure that additional explanations in description of extraGitoliteRc are necessary.

I think it's fine.

How do I properly add an assert to the module definition, to ensure that module options are compatible? I wanted to add assert (cfg.manageGitoliteRc || cfg.extraGitoliteRc == ""); to the definition of config after mkIf cfg.enable, but it resulted in infinite recursion. Currently this assert is in the definition of systemd.services."gitolite-init", which looks quite unnatural (it isn't about the gitolite-init service, it's about the entire module), but at least it works. Is there a better way? Is it possible to attach a error message to the assert?

Example NixOS assert:
https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/services/web-servers/lighttpd/default.nix#L212..L223

I'm confused by the way systemd's journal is shown. In my terminal (110 characters width), when the gitolite-init service issues the warning that I added, almost half of the terminal width is occupied with auxilary information, so I only see 50 characters for the actual message:
[...]
Unless I'm missing something and there is a better way to access systemd logs, it looks like I can't format the warning message in a way that would be convenient for most users. Do you have any recommendation of what to do here?

Perhaps reduce the size of the message. Maybe instead of having a large message, simply point users to "man configuration.nix" and the "manageGitoliteRc" option?

pvgoran · 2017-09-14T19:45:33Z

Example NixOS assert:
https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/services/web-servers/lighttpd/default.nix#L212..L223

Great! This is exactly what I was looking for. (Provided that it works in this case.)

Perhaps reduce the size of the message. Maybe instead of having a large message, simply point users to "man configuration.nix" and the "manageGitoliteRc" option?

This may work. I'll think about it.

I'll update the PR once more.

bjornfor · 2017-09-15T06:06:04Z

nixos/modules/services/misc/gitolite.nix

+        type = types.lines;
+        default = "";
+        example = literalExample ''
+          push( @{$RC{ENABLE}}, 'Kindergarten' );


Would you mind adding this line to the example?

$RC{UMASK} = 0027;

(To help out complete Perl dummies like me.)

Done. Also added some more helpful examples.

pvgoran · 2017-09-15T07:33:19Z

Now everything seems to be in order.

I also updated the commit/PR topic and description. Not sure if I should have added backticks to the commit description (github doesn't interpret them), but it's a small thing.

bjornfor · 2017-09-17T14:31:54Z

nixos/modules/services/misc/gitolite.nix

+             ( [[ -L "$GITOLITE_RC" ]] && [[ "$(readlink "$GITOLITE_RC")" =~ ^/nix/store/ ]] )
+          then
+        '' + rcSetupScriptIfDefaultFileOrStoreSymlink +
+        ''


Hm, this mixing of Nix and shell looks more complicate than necessary, I think. There is at least one if/else in Nix+sh that cancel each other out, if you take another look.

I think using this style when expanding nix values inside strings is more readable:

''hey, a nix value ${var} in the middle of the text''

IMHO, the added complexity of checking if .gitolite.rc is unchanged is not worth it. Personally, I'd say "given that manageGitoliteRc is set, is .gitolite.rc a symlink to nix store? If yes, update file, else complain."

If you make some of these changes, all of "rcDir, rcDirScript, rcAppendFile, rcAppendText" can become just "rcFile":

rcFile = runCommand "gitolite.rc" {} '' export HOME=gitolite-home mkdir -p gitolite-home/.gitolite/logs ${gitolite}/bin/gitolite print-default-rc > "$out" cat << EOF >> "$out" ${cfg.extraGitoliteRc} EOF ''

Love the documentation!

Hm, this mixing of Nix and shell looks more complicate than necessary, I think. There is at least one if/else in Nix+sh that cancel each other out, if you take another look.

Could you elaborate? I don't see it. Nix and shell conditions operate on different data.

I think using this style when expanding nix values inside strings is more readable ...

It is surely more readable, but if I put ${rcSetupScriptIfDefaultFileOrStoreSymlink} on a separate line inside a multi-line string, it will introduce an extra empty line after the contents of the variable, which doesn't look pretty. One could argue that readability of the source is more important that readability of the generated code, though...

IMHO, the added complexity of checking if .gitolite.rc is unchanged is not worth it.

I'm pretty sure it's important. The thing it, gitolite setup creates a default .gitolite.rc file if it doesn't exist. So all existing users of gitolite will already have it. And we don't want to bug them with warnings if they didn't actually make any customizations to the file, or make them wonder why their declarative configuration doesn't work, in case they add it and don't notice the warning.

all of "rcDir, rcDirScript, rcAppendFile, rcAppendText" can become just "rcFile":

Embedding ${cfg.extraGitoliteRc} into the shell script creates a danger of misinterpretation of its contents by the shell (if it has a lone EOF line, for some reason). My approach doesn't have this problem.

Could you elaborate? I don't see it. Nix and shell conditions operate on different data.

Perhaps I described it wrong. A concrete example of what I'm thinking of is replacing

'' + rcSetupScriptIfDefaultFileOrStoreSymlink +

(which has a long name and a if/else in its implementation)

with

${lib.optionalString cfg.manageGitoliteRc '' echo "warning message" # or echo "${warningMessage}" if you don't want multi-line shell code here in the .nix file ''}

In other words, I find some of the abstractions in this code too abstract. (But yes, that does probably add a blank line in case the condition is false.)

I'm pretty sure it's important. The thing it, gitolite setup creates a default .gitolite.rc file if it doesn't exist. So all existing users of gitolite will already have it. And we don't want to bug them with warnings if they didn't actually make any customizations to the file, or make them wonder why their declarative configuration doesn't work, in case they add it and don't notice the warning.

Good point. Though I figured now that there is a manageGitoliteRc option the warning message could be turned into an error, and that users perhaps better be made aware of the change from imperative to declarative config from the start. Even if they didn't make a change to the file. But OK, I get your reasoning.

Embedding ${cfg.extraGitoliteRc} into the shell script creates a danger of misinterpretation of its contents by the shell (if it has a lone EOF line, for some reason). My approach doesn't have this problem.

You can use echo '${foo}' > file if you don't like using heredoc. (You can also use a more special string than EOF. THIS_ENDS_THE_FILE for example?)

In other words, I find some of the abstractions in this code too abstract. (But yes, that does probably add a blank line in case the condition is false.)

While not ideal, I find that the approach (using named shell script fragments) is the best way (known to me) to convey the intent of the code. Embedding Nix if-then-else inside shell's if-then-else would look messy and confusing. (And we can't use optionalString instead of if-then-else, because shell syntax doesn't allow us to leave then- or else- branch empty.)

... the warning message could be turned into an error ...

Does this mean "exit with error", or just "write journal message with loglevel 3 instead of 4"?

You can use echo '${foo}' > file if you don't like using heredoc.

I found a way to properly escape it:

optionalString (cfg.extraGitoliteRc != "") '' echo ${escapeShellArg '' # Added by NixOS: ${removeSuffix "\n" cfg.extraGitoliteRc} # per perl rules, this should be the last line in such a file: 1; ''} >>"$out/gitolite.rc" '';

Does this look better? It's a bit too heavy with nested code-inside-strings, but it's somewhat shorter, and doesn't use additional variables.

Does this mean "exit with error", or just "write journal message with loglevel 3 instead of 4"?

Both :-)

[...] Does this look better?

IDK. I think both look kind of verbose. But I'm okay with either. (I would probably have written printf -- '%s' '${cfg.extraGitoliteRc}' >> "$out".)

Does this mean "exit with error", or just "write journal message with loglevel 3 instead of 4"?

Both :-)

I don't think we can do the former. It would break existing installations that have customized .gitolite.rc, I suppose this should not happen except in truly exceptional circumstances, right?

I don't mind raising the loglevel to provide better visibility, though. Also, maybe it's worth adding a note to the release information for 17.09, to make visibility even better? (I have no idea how this works, though. What is good enough to be added to release notes, how to do it, etc.)

I don't think we can do the former. It would break existing installations that have customized .gitolite.rc,

My thinking is that either we error out if .gitolite.rc is customized when upgrading to NixOS 17.09, or the manageGitoliteRc option would have to be turned off by default.

Hm, defaulting manageGitoliteRc to false is an easy way to not cause breakage...

Release notes for NixOS 17.09 is in nixos/doc/manual/release-notes/rl-1709.xml.

Hm, defaulting manageGitoliteRc to false is an easy way to not cause breakage...

This will not fulfill your original intent of making users "aware of the change from imperative to declarative config from the start". :) And it feels less "natural" for NixOS.

Leaving manageGitoliteRc defaulting to true would at least issue the warning.

Release notes for NixOS 17.09 is in nixos/doc/manual/release-notes/rl-1709.xml.

Do I just prepare a PR for them as well? Should it be a separate PR, or it needs to be included in this one?

This will not fulfill your original intent of making users "aware of the change from imperative to declarative config from the start". :)

Well, there is less need to be aware of something if it doesn't happen :-)

And it feels less "natural" for NixOS.

Agreed. But since it is a breaking change, we could let the transition go over one release cycle instead of introducing breakage right away. So for NixOS 17.09 the manageGitoliteRc option (=false) is introduced (and mentioned in the release notes). Then for NixOS 18.03 the default can be changed.

Do I just prepare a PR for them as well? Should it be a separate PR, or it needs to be included in this one?

A separate commit in this PR is preferred.

pvgoran · 2017-09-18T21:55:19Z

For now, I got rid of extra variables for rcDirScript, and raised the log level to 3 for the initialization script warning.

bjornfor

Make manageGitoliteRc default to false (option default can be changed in next release, but for now be gentle). Update text that is affected by that change. Adding release note entry is optional if doing it this way, since there is no breaking change.

UPDATE: Oops, I meant "Make manageGitoliteRc default to FALSE" obviously.

pvgoran · 2017-09-19T08:19:13Z

@bjornfor Sorry, I still don't understand the upgrade strategy you have in mind.

Don't do a breaking change now, and do it on 18.03 instead? To me, this looks just as unacceptable as doing a breaking change now. (Users are not guaranteed to notice the new options/functionality until a breaking change is made. Also, users can upgrade right from 17.03 to 18.03.)
Don't set manageGitoliteRc to true by default now, and do it on 18.03 instead, while keeping (now and in the future) the current "soft" (warning-only) reaction to conflict between user's .gitolite.rc and manageGitoliteRc=true? I don't see what good can be achieved this way.
Something other?

bjornfor · 2017-09-19T15:56:30Z

Don't do a breaking change now, and do it on 18.03 instead? To me, this looks just as unacceptable as doing a breaking change now. (Users are not guaranteed to notice the new options/functionality until a breaking change is made. Also, users can upgrade right from 17.03 to 18.03.)

Reasoning:

I think it's more likely that users upgrade to every stable NixOS release than skip some.
If users upgrade now, and read the release notes(?) they have 6 months to decide what to do with the manageGitoliteRc option.
In 6 months we may even decide that leaving the manageGitoliteRc option default off is fine. For instance, users.mutableUsers still defaults to true. (I'm guessing it'll change in the future, but for now that too defaults to "same as before, impure".)

Don't set manageGitoliteRc to true by default now, and do it on 18.03 instead, while keeping (now and in the future) the current "soft" (warning-only) reaction to conflict between user's .gitolite.rc and manageGitoliteRc=true? I don't see what good can be achieved this way.

No, I don't think that's a good idea.

Possible exception: if there was a way to differentiate between a config having the NixOS default AND explicitly setting the option to true/false in own config. I.e., if we could gently "push" users to make a choice. But I don't think that's possible.

pvgoran · 2017-09-20T10:05:50Z

My strong opinion is that we can't do changes that will break users' installations "by default" (without them doing anything other than upgrading), no matter how far in the future. (Unless it can't be avoided, or we're talking about something really prominent and important for everyone, like mutableUsers you mentioned. Clearly we don't have this case here.) As a user I would really dislike to see such behaviour.

if there was a way to differentiate between a config having the NixOS default AND explicitly setting the option to true/false in own config (...) But I don't think that's possible.

This is possible (unless I misunderstand you here): I can make the manageGitoliteRc option tri-state by using the type types.nullOr types.bool. By default it would be null (the user didn't make a choice), and it can be explicitly set to either true or false.

With this, we can have a broader array of behaviour in case there are custom modifications. For example, if manageGitoliteRc is explicitly set to true or extraGitoliteRc is specified, then error out of the initialization script; if manageGitoliteRc is not set, then just issue a warning and continue without overwriting user's customizations or breaking his installation.

In case of no custom modifications, the .gitolite.rc file would be replaced with a NixOS-managed symlink by default (that is, unless manageGitoliteRc is explicitly set to false).

An alternative variant would be to let effectiveManageGitoliteRc = (cfg.manageGitoliteRc == true || cfg.extraGitoliteRc != "") and use this value to decide whether we manage the file or not. This way, we can have an error if there is a conflict between declarative options and customizations in the file, without breaking existing installations by default. It's also simpler than the variant above. However, this way we will never have .gitolite.rc managed by default, and the users will not get any notification/warning about the new declarative options.

bjornfor · 2017-09-20T15:37:58Z

I just noticed I had written the exact opposite of what I meant, in the review status. Based on the context I obviously meant to suggest that manageGitoliteRc is set to FALSE by default, so that we DON'T break existing users setup. (Sorry for the confusion.)

Good point about the tristate option.

I'd prefer to just make a minimum implementation now for NixOS 17.09. That is, boolean manageGitoliteRc defaulting to false. Deciding on upgrade path can be done later (if at all needed).

What do you think?

pvgoran · 2017-09-20T16:50:23Z

In fact, if manageGitoliteRc is false by default, this option becomes redundant. We can just manage the file if extraGitoliteRc is specified, and not manage the file if extraGitoliteRc is not specified. The only useful application of manageGitoliteRc would be to silence the warning and/or to allow to keep the legacy behaviour, by explicitly setting it to false.

pvgoran · 2017-09-20T16:58:01Z

I'd prefer to just make a minimum implementation now for NixOS 17.09.

This makes sense. So, if you don't think that the currently implemented approach (manageGitoliteRc enabled by default, with a warning in case of conflict) is appropriate, I'll remove this option as per my previous comment.

bjornfor · 2017-09-20T18:41:30Z

Yes, please remove manageGitoliterc option for now.

pvgoran · 2017-09-20T21:09:47Z

Done.

bjornfor · 2017-09-21T05:50:00Z

@pvgoran: Tested and ready to go? If so, then I'll add it to master and backport to release-17.09.

bjornfor · 2017-09-21T05:50:32Z

Thanks for writing such good documentation in the NixOS option!

pvgoran · 2017-09-21T07:38:25Z

Tested and ready to go?

Yes, and I just tested it again.

bjornfor · 2017-09-21T18:56:38Z

@pvgoran: Git author is "Someone someone@localhost". Sure you don't want something else here?

NeQuissimus · 2017-09-21T19:04:16Z

Or maybe someone else? (I'll show myself out...)

Add the `extraGitoliteRc` option to customize the `.gitolite.rc` configuration file declaratively. Resolves NixOS#29249.

pvgoran · 2017-09-22T02:28:33Z

Ok, I resolved my identity crisis with a new commit. :)

bjornfor · 2017-09-22T16:31:24Z

Pushed to master (c73a381) and cherry-pick'ed to release-17.09 (5b1d686). Thank you!

bjornfor reviewed Sep 13, 2017

View reviewed changes

bjornfor requested changes Sep 13, 2017

View reviewed changes

pvgoran force-pushed the gitolite-extraGitoliteRc branch from 13625d9 to d536eb4 Compare September 14, 2017 17:12

bjornfor reviewed Sep 15, 2017

View reviewed changes

pvgoran force-pushed the gitolite-extraGitoliteRc branch from d536eb4 to d10aafb Compare September 15, 2017 07:20

pvgoran changed the title ~~nixos/gitolite: add extraGitoliteRc option~~ nixos/gitolite: manage .gitolite.rc declaratively Sep 15, 2017

pvgoran force-pushed the gitolite-extraGitoliteRc branch from d10aafb to d37d49e Compare September 15, 2017 07:28

bjornfor reviewed Sep 17, 2017

View reviewed changes

bjornfor approved these changes Sep 17, 2017

View reviewed changes

pvgoran force-pushed the gitolite-extraGitoliteRc branch 2 times, most recently from 878c7c6 to b569983 Compare September 18, 2017 21:51

bjornfor requested changes Sep 19, 2017

View reviewed changes

pvgoran force-pushed the gitolite-extraGitoliteRc branch from b569983 to 02833e0 Compare September 20, 2017 21:08

pvgoran changed the title ~~nixos/gitolite: manage .gitolite.rc declaratively~~ nixos/gitolite: customize .gitolite.rc declaratively Sep 20, 2017

bjornfor approved these changes Sep 21, 2017

View reviewed changes

nixos/gitolite: customize .gitolite.rc declaratively

d6e496a

Add the `extraGitoliteRc` option to customize the `.gitolite.rc` configuration file declaratively. Resolves NixOS#29249.

pvgoran force-pushed the gitolite-extraGitoliteRc branch from 02833e0 to d6e496a Compare September 22, 2017 02:25

bjornfor added 0.kind: enhancement 6.topic: nixos labels Sep 22, 2017

bjornfor closed this Sep 22, 2017

pvgoran deleted the gitolite-extraGitoliteRc branch September 27, 2017 05:20

nixos/gitolite: customize .gitolite.rc declaratively #29295

nixos/gitolite: customize .gitolite.rc declaratively #29295

Conversation

pvgoran commented Sep 13, 2017 • edited

Things done

mention-bot commented Sep 13, 2017

pvgoran commented Sep 13, 2017

bjornfor commented Sep 13, 2017

bjornfor commented Sep 13, 2017

pvgoran commented Sep 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjornfor commented Sep 13, 2017

Choose a reason for hiding this comment

pvgoran Sep 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjornfor commented Sep 13, 2017

pvgoran commented Sep 13, 2017

bjornfor commented Sep 13, 2017

bjornfor commented Sep 13, 2017

bjornfor left a comment

Choose a reason for hiding this comment

pvgoran commented Sep 13, 2017

bjornfor commented Sep 13, 2017

pvgoran commented Sep 14, 2017

bjornfor commented Sep 14, 2017

pvgoran commented Sep 14, 2017

bjornfor commented Sep 14, 2017

pvgoran commented Sep 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvgoran commented Sep 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvgoran commented Sep 18, 2017

bjornfor left a comment • edited

Choose a reason for hiding this comment

pvgoran commented Sep 19, 2017

bjornfor commented Sep 19, 2017

pvgoran commented Sep 20, 2017

bjornfor commented Sep 20, 2017

pvgoran commented Sep 20, 2017

pvgoran commented Sep 20, 2017

bjornfor commented Sep 20, 2017

pvgoran commented Sep 20, 2017

bjornfor commented Sep 21, 2017

bjornfor commented Sep 21, 2017

pvgoran commented Sep 21, 2017

bjornfor commented Sep 21, 2017

NeQuissimus commented Sep 21, 2017

pvgoran commented Sep 22, 2017 • edited

bjornfor commented Sep 22, 2017

pvgoran commented Sep 13, 2017 •

edited

pvgoran Sep 13, 2017 •

edited

bjornfor left a comment •

edited

pvgoran commented Sep 22, 2017 •

edited