huginn: init at 2016-09-14 #18590

mogorman · 2016-09-14T16:39:30Z

Motivation for this change

This is a new package huginn, http://github.com/cantino/huginn , it is an agent platform. This is my first service module, as well as my first ruby package. I would appreciate people looking it over. this pr also depends on #18361 or it can't be built due to unicode ruby files in the dependencies. I would also like to backport this into 16.09 after master if someone could show me how.

Things done

Tested using sandboxing
(nix.useSandbox on NixOS,
or option build-use-sandbox in nix.conf
on non-NixOS)
Built on platform(s)
- [x ] NixOS
- OS X
- Linux
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 · 2016-09-14T16:39:33Z

@mogorman, thanks for your PR! By analyzing the annotation information on this pull request, we identified @edolstra, @joachifm and @offlinehacker to be potential reviewers

mogorman · 2016-09-14T16:43:27Z

fixed merge conflict

mogorman · 2016-09-20T14:47:06Z

bumped id number again to fix merge conflict

joachifm · 2016-09-22T12:38:50Z

What's up with the need to add ~3,600 lines of code to the main repo for a single package and a service?

mogorman · 2016-09-22T14:13:49Z

the majority of the bloat was just defining the config file completely so it can be easily configured by nix. it isn't really bloat is it?

Mic92 · 2016-10-10T22:22:57Z

pkgs/applications/misc/huginn/Gemfile

@@ -0,0 +1,169 @@
+Encoding.default_external = Encoding::UTF_8


This file could be probably replaced by

source "https://rubygems.org" gem 'huginn', git: "https://github.com/cantino/huginn.git", ref: '005be58d30181b1e0ca5b29cc9a8729ee0d7bcab'

ok, this will not work, because you apply patches.

Mic92 · 2016-10-10T22:27:35Z

nixos/modules/services/misc/huginn.nix

+      preStart = ''
+        rm -rf ${cfg.statePath}/tmp
+        mkdir -p ${cfg.statePath}/log
+        mkdir -p ${cfg.statePath}/tmp/pids


can be replaced by a single mkdir:

mkdir -p ${cfg.statePath}/log ${cfg.statePath}/tmp/{pids,cache,sockets}

Mic92 · 2016-10-10T22:28:47Z

nixos/modules/services/misc/huginn.nix

+        mkdir -p ${cfg.statePath}/tmp/sockets
+
+        touch ${cfg.statePath}/tmp/secrets
+	if [ "${cfg.appSecretToken}" = "" ]; then


In general, can you fix the indentation?

Mic92 · 2016-10-10T22:31:01Z

nixos/modules/services/misc/huginn.nix

+            ${config.services.postgresql.package}/bin/createdb --owner ${cfg.databaseUser} ${cfg.databaseName} huginn  || true
+	    if [ "${cfg.databasePassword}" = "" ]; then
+	      echo "creating db"
+              ${huginn-rake}/bin/huginn-rake db:create RAILS_ENV=production APP_SECRET_TOKEN="null" DATABASE_PASSWORD=`cat ${cfg.statePath}/db-password`


rake accepts multiple tasks at once: ${huginn-rake}/bin/huginn-rake db:create db:migrate db:seed

Mic92 · 2016-10-10T22:32:18Z

nixos/modules/services/misc/huginn.nix

+
+in {
+
+  options = {


wow, many options here. This could be split into a separate file probably

Mic92 · 2016-10-10T22:34:30Z

pkgs/applications/misc/huginn/default.nix

+    cp -r . $out/share/huginn
+    cp ${gemfile} $out/share/huginn/Gemfile
+    cp ${lockfile} $out/share/huginn/Gemfile.lock
+#I dont know why these need to exist but what can i say


here you could also use a single mkdir

Mic92 · 2016-10-10T22:35:27Z

pkgs/applications/misc/huginn/default.nix

+
+  installPhase = ''
+    export LD_LIBRARY_PATH=${curl.out}/lib:${postgresql}/lib
+    RAILS_ENV=production APP_SECRET_TOKEN=REPLACE_ME_NOW! DATABASE_ADAPTER=nulldb HUGINN_STATE_PATH=tmp/ SKIP_STORAGE_VALIDATION=true bundle exec rake assets:precompile


this could be break into multiple lines using \ for readability.

Mic92 · 2016-10-10T22:39:14Z

nixos/modules/services/misc/huginn.nix

+    ASSET_HOST=cfg.assetHost;
+    FORCE_SSL= (if cfg.forceSSL then "true" else "false");
+    INVITATION_CODE=cfg.registration.invitationCode;
+    SKIP_INVITATION_CODE=(if cfg.registration.skipInvitation then "true" else "false");


maybe replace by a helper function:

toBool = option : if option then "true" else "false"; ... SKIP_INVITATION_CODE=(toBool cfg.registration.skipInvitation);

Mic92 · 2016-10-10T22:45:55Z

nixos/modules/services/misc/huginn.nix

+	      echo "migrating db"
+	      ${huginn-rake}/bin/huginn-rake db:migrate RAILS_ENV=production APP_SECRET_TOKEN="null"
+	      echo "seeding db"
+	      ${huginn-rake}/bin/huginn-rake db:seed RAILS_ENV=production APP_SECRET_TOKEN="null" SEED_USERNAME=${cfg.initialUser} SEED_PASSWORD=${cfg.initialPassword} SKIP_INVITATION_CODE=true REQUIRE_CONFIRMED_EMAIL=false


what happens if you pass DATABASE_PASSWORD= to rake? If this works you could save the if expression.

Mic92 · 2016-10-10T22:54:26Z

nixos/modules/services/misc/huginn.nix

+	if [ "${cfg.appSecretToken}" = "" ]; then
+	  printf "APP_SECRET_TOKEN=" >>  ${cfg.statePath}/tmp/secrets
+          tr -dc A-Za-z0-9 < /dev/urandom 2> /dev/null | head -c 32 >> ${cfg.statePath}/tmp/secrets
+	  echo "" >> ${cfg.statePath}/tmp/secrets


printf 'APP_SECRET_TOKEN=%s\n' "$(tr -dc A-Za-z0-9 < /dev/urandom 2> /dev/null|head -c32)" > ${cfg.statePath}/tmp/secrets

maybe this could be also wrapped and reused in a shell function

gen_secret() { printf "$1=%s\n" "$(tr -dc A-Za-z0-9 < /dev/urandom 2> /dev/null|head -c32)" > "$2" } gen_secret "APP_SECRET_TOKEN" "${cfg.statePath}/tmp/secrets"

ocharles · 2017-03-17T20:52:03Z

Hi @mogorman, what is the status of this pull request? It looks like @Mic92 has given you a thorough code review.

bachp · 2017-08-27T09:07:49Z

@mogorman I would also like to see this finished. Do you need any help?

disassembler · 2017-11-01T00:15:01Z

Closing this out since there's been no response for over a year. @bachp and @ocharles please open a new PR for this if you're interested in working on this.

CMCDragonkai · 2018-04-04T03:32:18Z

This would really be nice to have. Might work on it later if nobody picks this up.

mogorman force-pushed the huginn branch from 3b56b6e to 5a1a7d3 Compare September 14, 2016 16:43

LnL7 added the 2.status: merge conflict label Sep 17, 2016

mogorman force-pushed the huginn branch from 5a1a7d3 to 642c062 Compare September 20, 2016 14:18

huginn: init at 2016-09-14

5c5c952

mogorman force-pushed the huginn branch from 642c062 to 5c5c952 Compare September 21, 2016 14:02

Mic92 reviewed Oct 10, 2016

View reviewed changes

nixos/modules/services/misc/huginn.nix

in {

options = {

Copy link

Member

Mic92 Oct 10, 2016

Choose a reason for hiding this comment

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

wow, many options here. This could be split into a separate file probably

Mic92 reviewed Oct 10, 2016

View reviewed changes

vrthra added 0.kind: enhancement 8.has: package (new) labels Feb 11, 2017

disassembler closed this Nov 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

huginn: init at 2016-09-14 #18590

huginn: init at 2016-09-14 #18590

mogorman commented Sep 14, 2016

mention-bot commented Sep 14, 2016

mogorman commented Sep 14, 2016

mogorman commented Sep 20, 2016

joachifm commented Sep 22, 2016

mogorman commented Sep 22, 2016

Mic92 Oct 10, 2016

Mic92 Oct 10, 2016

Mic92 Oct 10, 2016

Mic92 Oct 10, 2016

Mic92 Oct 10, 2016

Mic92 Oct 10, 2016

Mic92 Oct 10, 2016

Mic92 Oct 10, 2016

Mic92 Oct 10, 2016

Mic92 Oct 10, 2016

Mic92 Oct 10, 2016

Mic92 Oct 10, 2016

ocharles commented Mar 17, 2017

bachp commented Aug 27, 2017

disassembler commented Nov 1, 2017

CMCDragonkai commented Apr 4, 2018

		@@ -0,0 +1,169 @@
		Encoding.default_external = Encoding::UTF_8

huginn: init at 2016-09-14 #18590

huginn: init at 2016-09-14 #18590

Conversation

mogorman commented Sep 14, 2016

Motivation for this change

Things done

mention-bot commented Sep 14, 2016

mogorman commented Sep 14, 2016

mogorman commented Sep 20, 2016

joachifm commented Sep 22, 2016

mogorman commented Sep 22, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ocharles commented Mar 17, 2017

bachp commented Aug 27, 2017

disassembler commented Nov 1, 2017

CMCDragonkai commented Apr 4, 2018