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

Improved NixOS module for Hydra (WIP) #501

Closed
wants to merge 17 commits into from
Closed

Conversation

@taktoa
Copy link
Contributor

taktoa commented Aug 3, 2017

I'm working on making the Hydra configuration more declarative; I think the current state of affairs where you literally have to reverse-engineer the Perl source to figure out what options you can give in services.hydra.extraConfig is embarrassing; you'd think that Hydra, of all projects, would be really usable with NixOS.

My ultimate plan is what I call "Fully Declarative Hydra": every aspect of Hydra's state should be specified in the NixOS module except the build and evaluation history.

@@ -64,7 +67,7 @@ with rec {
};

package = mkOption {
type = types.path;
type = types.package;

This comment has been minimized.

Copy link
@taktoa

taktoa Aug 3, 2017

Author Contributor

The NixOS module documentation recommends types.package over types.path, but there may be some people taking advantage of the current state of affairs; feel free to let me know if this is the case.

Copy link
Member

domenkozar left a comment

This essentially (minus all the styling changes that are hard to track) adds following NixOS options for Hydra:

  • googleClientID
  • private
  • storeMode

Obviously needs documentation :)

env = {
NIX_REMOTE = "daemon";
# FIXME: why isn't this removed yet? we've been on 17.03 for a while now
SSL_CERT_FILE = "/etc/ssl/certs/ca-certificates.crt"; # Remove in 16.03

This comment has been minimized.

Copy link
@domenkozar

domenkozar Aug 7, 2017

Member

Let's remove it.

@taktoa
Copy link
Contributor Author

taktoa commented Aug 7, 2017

@domenkozar I have a lot more options added locally; I'll push later today

@Ericson2314
Copy link
Member

Ericson2314 commented Aug 7, 2017

We could do the indentation modernization in a separate PR too just to get that out of the way.

@taktoa
Copy link
Contributor Author

taktoa commented Aug 9, 2017

@domenkozar: I just sent up all the stuff I've written; it's messy and probably doesn't evaluate yet

});

types = lib.types // {
oneof = list: (

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 Aug 25, 2017

It seems like oneof is not used anywhere else within the pull request

Also, oneof seems very similar to types.enum

This comment has been minimized.

Copy link
@taktoa

taktoa Aug 25, 2017

Author Contributor

oneof is not the same as enum, as enum cannot take any NixOS option type; it can only take values.

};

{
foldr = lib.lists.fold;

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 Aug 25, 2017

nixpkgs masteralready hasfoldr`

This comment has been minimized.

Copy link
@taktoa

taktoa Aug 25, 2017

Author Contributor

Yeah, I was testing this on a nixpkgs version without foldr.


{
foldr = lib.lists.fold;
foldr1 = foldToFold1 lists.foldr;

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 Aug 25, 2017

All of the fold*1 utilities seem like they are unused in this pull request. The only exception is foldr1 but that is used within oneof which is in turn not used elsewhere

foldr1 = foldToFold1 lists.foldr;
foldl = lib.lists.foldl;
foldl1 = foldToFold1 lists.foldl;
foldl' = lib.lists.foldl';

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 Aug 25, 2017

Why does this add foldl' to lib.lists when it's already an attribute of lib.lists? Same comment for foldl

forceDeep = x: builtins.deepSeq x x;

strings = lib.strings // {
intercalate = str: list: lib.concatStrings (lib.intersperse str list);

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 Aug 25, 2017

intercalate already exists in the standard library as lib.concatMapStringsSep

This comment has been minimized.

Copy link
@taktoa

taktoa Aug 25, 2017

Author Contributor

Good to know!


forceWHNF = x: builtins.seq x x;
forceDeep = x: builtins.deepSeq x x;

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 Aug 25, 2017

This pull request does not use forceWHNF and forceDeep

Also forceWHNF does not do anything because seq x x = x

This comment has been minimized.

Copy link
@taktoa

taktoa Aug 25, 2017

Author Contributor

Good point on forceWHNF.

@@ -62,15 +130,16 @@ in
};

package = mkOption {
type = types.path;
#default = pkgs.hydra;
type = types.package;

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 Aug 25, 2017

You should probably avoid making unrelated breaking changes when submitting a large PR, because if this change breaks anything we can't easily roll back


};
googleClientID = mkOption {

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 Aug 25, 2017

Is this googleClientID a secret or is it safe to store in the configuration?

This comment has been minimized.

Copy link
@taktoa

taktoa Aug 25, 2017

Author Contributor

I think it is a secret.

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 Sep 2, 2017

Would it be possible to template the Hydra configuration at run-time during the systemd.services.hydra-init.preStart by reading the secret from a preconfigured system path (i.e. /etc/hydra/googleClientID or maybe something under /run)? That way you don't need to worry about the secret being publicly readable inside the /nix/store and the secret key file can have appropriate permissions set so that only the hydra daemon can read the file

(Same question for any other secrets these changes may require)

This comment has been minimized.

Copy link
@taktoa

taktoa Sep 4, 2017

Author Contributor

This is not a bad idea. Will investigate.

This comment has been minimized.

Copy link
@ElvishJerricco

ElvishJerricco Mar 2, 2018

+1. I think it's currently impossible to configure Hydra with the current NixOS module without putting e.g. github auth tokens in the nix store.

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 Mar 2, 2018

@ElvishJerricco: I have an example NixOps deploy of Hydra that patches Hydra to fix this. You can see the deploy here:

https://github.com/dhall-lang/dhall-lang/tree/master/nixops

... and the patch here:

https://github.com/dhall-lang/dhall-lang/blob/master/nixops/hydra.patch

default = 100;
example = 1000;
description = ''
FIXME: lol

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 Aug 25, 2017

Did you intend to keep this FIXME? :)


};
googleClientID = mkOption {

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 Sep 2, 2017

Would it be possible to template the Hydra configuration at run-time during the systemd.services.hydra-init.preStart by reading the secret from a preconfigured system path (i.e. /etc/hydra/googleClientID or maybe something under /run)? That way you don't need to worry about the secret being publicly readable inside the /nix/store and the secret key file can have appropriate permissions set so that only the hydra daemon can read the file

(Same question for any other secrets these changes may require)

parameter key.
'';
# FIXME: ensure that the above is actually true
# https://github.com/NixOS/nix/blob/2fd8f8bb99a2832b3684878c020ba47322e79332/src/libstore/store-api.hh#L693-L718

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 Sep 2, 2017

Could you create a GitHub issue against the nix repository for this and link that issue here?

group = "hydra";
useDefaultShell = true;
home = "${baseDir}/queue-runner"; # really only to keep SSH happy
storeURI = mkOption {

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 Sep 2, 2017

It looks like this option is not used yet

# then you should set `auth = "foo";`.
#
# There can be multiple <githubstatus> stanzas.
makeGithubStatusStanza = (

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 Sep 2, 2017

It looks like these make*Stanza are not used yet?

This comment has been minimized.

Copy link
@taktoa

taktoa Sep 4, 2017

Author Contributor

the intention is to fold the semantics defined by these functions into hydra-module.nix and then delete temporary.nix altogether

@taktoa
Copy link
Contributor Author

taktoa commented Sep 4, 2017

Related issue: NixOS/nix#1556

taktoa added 17 commits Aug 2, 2017
@taktoa taktoa force-pushed the taktoa:taktoa/module branch from 4dbe2ea to 8a714cf Sep 17, 2017
@domenkozar
Copy link
Member

domenkozar commented Oct 8, 2017

@taktoa sorry I just have a bit of time over weekends, is this ready for review?

@taktoa
Copy link
Contributor Author

taktoa commented Oct 8, 2017

Sorry, not yet, I've been busy with other things.

@dtzWill
Copy link
Contributor

dtzWill commented Feb 20, 2018

Gentle but enthusiastic ping! :)

@ElvishJerricco
Copy link

ElvishJerricco commented Mar 2, 2018

Seconding that ping!

@taktoa
Copy link
Contributor Author

taktoa commented Mar 2, 2018

yep, was actually working on this a couple days ago

@gilligan
Copy link
Contributor

gilligan commented May 11, 2020

@taktoa this hasn’t been touched in a while. I think several people had shown interest in this and you apparently invested quite some work. Do you have any intentions to pick this up again at some point?

Maybe you could try to break this down instead into smaller changes that are easier and more feasible to get done and merged?

It had been 2 years but maybe there is still some interest on your side.

@taktoa
Copy link
Contributor Author

taktoa commented May 11, 2020

Sorry, but I just don't have the cycles these days. I'd be happy to let someone take this work over.

@taktoa taktoa closed this May 11, 2020
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

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