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

nixos gerrit module #82929

Merged
merged 2 commits into from Mar 26, 2020
Merged

nixos gerrit module #82929

merged 2 commits into from Mar 26, 2020

Conversation

@zimbatm
Copy link
Member

zimbatm commented Mar 19, 2020

Motivation for this change

A convenient way to manage gerrit on a NixOS machine.

This PR also adds a git config generator in the lib/ folder.

This PR was highly inspired by #82137

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@zimbatm zimbatm mentioned this pull request Mar 19, 2020
3 of 7 tasks complete
nixos/modules/services/web-apps/gerrit.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/gerrit.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/gerrit.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/gerrit.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/gerrit.nix Outdated Show resolved Hide resolved
toGitINI = attrs:
with builtins;
let
# create [section "subsection"] keys from "section.subsection" attrset names

This comment has been minimized.

Copy link
@edef1c

edef1c Mar 19, 2020

Member

I don't particularly approve of the in-band signalling here. I'd rather have #82137's style of section generation.

This comment has been minimized.

Copy link
@zimbatm

zimbatm Mar 19, 2020

Author Member

I wasn't sure if there would be other sections with that type of notation. At the moment the nixos module only configures gerrit.config. I had in mind that it could also be used to generate the other config files. For example I have this one:

its/actions-its-jira.config

# https://gerrit.googlesource.com/plugins/its-base/+/master/src/main/resources/Documentation/config-rulebase-common.md
[rule "open"]
    event-type = patchset-created
    action = add-standard-comment
[rule "merged"]
    event-type = change-merged
    action = add-standard-comment
[rule "abandoned"]
    event-type = change-abandoned
    action = add-standard-comment
[rule "restored"]
    event-type = change-restored
    action = add-standard-comment

This comment has been minimized.

Copy link
@edef1c

edef1c Mar 19, 2020

Member

Note that I'm not complaining about the module options themselves — I'm quite happy with the new input format, and had been intending to refactor pluginSettings away myself. My problem here is with the in-band signalling inside the formatting code.

This comment has been minimized.

Copy link
@zimbatm

zimbatm Mar 19, 2020

Author Member

My bad, I am not familiar with the meaning of "in-band signalling". Did you mean that the comment should be moved up to the function definition? If yes then I agree :)

This comment has been minimized.

Copy link
@zimbatm

zimbatm Mar 19, 2020

Author Member

fixed

This comment has been minimized.

Copy link
@edef1c

edef1c Mar 27, 2020

Member

By "in-band signalling", I mean that data and framing information aren't kept separate — I should be able to express an attribute named "foo.bar" distinctly from a nested attribute set foo.bar. Just joining and splitting the strings without an escaping mechanism doesn't work for that. My prior implementation did not have this issue.

This comment has been minimized.

Copy link
@zimbatm

zimbatm Mar 27, 2020

Author Member

Hmm ok. I will take another look on monday then and make another PR

@zimbatm

This comment has been minimized.

Copy link
Member Author

zimbatm commented Mar 22, 2020

@edef1c what do you think of the state of this PR? I think it needs some more documentation but am afraid to touch the XML :)

@flokli

This comment has been minimized.

Copy link
Contributor

flokli commented Mar 25, 2020

I added a NixOS vm test which pokes both HTTP and SSH ports.

It'd be really nice if we could exercise the API a bit and actually run some commands through the ssh CLI, but other than setting auth.type = "DEVELOPMENT_BECOME_ANY_ACCOUNT" and clicking in the web interface, I couldn't find a way to programmatically set the admin API credentials and configure a ssh pubkey.

zimbatm and others added 2 commits Mar 5, 2020
This code was taken from the home-manager project.
Co-authored-by: edef <edef@edef.eu>
Co-authored-by: Florian Klink <flokli@flokli.de>
@zimbatm zimbatm force-pushed the zimbatm:nixos-gerrit branch from fbc8a7b to d37a0dc Mar 26, 2020
@zimbatm zimbatm merged commit d8f0c54 into NixOS:master Mar 26, 2020
13 checks passed
13 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
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
@zimbatm zimbatm deleted the zimbatm:nixos-gerrit branch Mar 26, 2020
multipleType = either primitiveType (listOf primitiveType);
sectionType = lazyAttrsOf multipleType;
supersectionType = lazyAttrsOf (either multipleType sectionType);
in lazyAttrsOf supersectionType;

This comment has been minimized.

Copy link
@Infinisil

Infinisil Mar 27, 2020

Member

lazyAttrsOf should ideally only be used if either mkIf false definitions don't make sense (which isn't the case here), or if such definitions are handled with an emptyValue that then gets filtered out (e.g. by using lazyAttrsOf (nullOr ...), then filtering out null's before generating the file). Because this isn't done here, the following will error:

{
  services.gerrit.settings.foo.bar = mkIf false 10;
}
error: The option `services.gerrit.settings.foo.bar' is used but not defined.

This comment has been minimized.

Copy link
@Infinisil

Infinisil Mar 27, 2020

Member

Or alternatively just using attrsOf instead (which then doesn't support values to depend on other values, which is probably fine here)

This comment has been minimized.

Copy link
@zimbatm

zimbatm Mar 27, 2020

Author Member

So just replace all the lazyAttrsOf with attrsOf? I must admit that I cargo-culted this and don't have a good understanding of these distinctions even after your explanation.

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

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.