Fix Janitor configuration; #207

bnjbvr · 2018-01-16T22:38:56Z

Hey @jankeromnes, can you help me please? It's almost working, but I couldn't find out how to start the postgresql daemon from supervisord. Any idea here?

edit: starting from supervisord as I did doesn't work, for some reason (?).

jankeromnes · 2018-01-16T23:06:36Z

Wow, super cool! Thanks for working on this fix. 😄

I also don't see anything wrong with your approach. Have you looked at sudo docker logs <your-test-container>? This will display the supervisor logs with any errors postresql might throw.

bnjbvr · 2018-01-16T23:07:18Z

Actually, scratch my issue: by cargo culting https://github.com/allisson/docker-postgresql/blob/master/supervisord.conf I've found a way that works.

bnjbvr · 2018-01-16T23:53:54Z

This is ready for review:

@jankeromnes can you have a look at the Janitor changes, please?
@Chocobozzz too if you want; there's also a change to the watch:client command so that the front web server is exposed to all interfaces (viz. to the docker sub network). I think it's pretty safe.

Thanks!

jankeromnes

Thanks a lot for making this work! I have a few comments, but generally I'm fine with these changes if Chocobozzz is too 😄

jankeromnes · 2018-01-17T00:01:29Z

support/docker/dev/Dockerfile

 # Configure Janitor for PeerTube.
 ADD janitor.json /home/user/
 RUN sudo chown user:user /home/user/janitor.json

 # Configure and build PeerTube.
-RUN yarn install \
- && npm run build


Nit: Why do you no longer pre-build PeerTube?

npm run dev will build the client on the fly

What @Chocobozzz said. Closing.

jankeromnes · 2018-01-17T00:03:51Z

support/docker/dev/Dockerfile

+ADD create_user.sql /tmp/
+RUN sudo service postgresql start && \
+    sudo -u postgres psql --file=/tmp/create_user.sql && \
+    sudo rm /tmp/create_user.sql


Nit: Even if you delete files here, they'll remain part of the image history, since they were ADDed above. I suggest leaving this file somewhere useful, or running these steps with inline psql commands as opposed to a file.

Also, if this is really necessary to get PeerTube to work, why isn't it in a Makefile or in the package.json NPM scripts?

Re: files: well, these are tiny, so I don't seem to care too much ;) I'll just let them in /tmp in this case.

Re: Makefile/package.json docs: it depends a lot on your database install / version; it's more of an external dependency that something mandated by PeerTube, so IMO it makes sense to keep it separate.

jankeromnes · 2018-01-17T00:07:58Z

support/docker/dev/janitor.json

+      "label": "PeerTube API",
+      "proxy": "https"
+    },
+    "3000": {


Super-nit: Please sort ports from smallest to largest (i.e. "3000" before "9000").

jankeromnes · 2018-01-17T00:08:51Z

support/docker/dev/janitor.json

+      "proxy": "https"
+    },
+    "3000": {
+      "label": "PeerTube front",


Nit: What about "web app" instead of "front"? I'd find that more explicit/transparent.

If I wanted to be picky, I'd say the API also uses web technologies and is an app, so it's also a web app ;) But yeah, it probably makes more sense overall :)

jankeromnes · 2018-01-17T00:09:23Z

support/docker/dev/janitor.json

      "proxy": "https",
      "preview": true
    }
  },
  "scripts": {
-    "Start PeerTube": "npm start",
+    "Start PeerTube": "npm run dev",


Interesting, why did you need to switch this? What's the difference between npm start and npm run dev?

npm start just serves the built server and the compiled client files. It's for production, after anpm run build.

npm run dev builds the server, run it on port 9000, builds the client on the fly and serves it on port 3000

jankeromnes · 2018-01-17T00:10:49Z

support/docker/dev/supervisord.conf

@@ -0,0 +1,3 @@
+[program:postgresql]
+user = user
+command = sudo -u postgres /usr/lib/postgresql/9.5/bin/postgres -D /var/lib/postgresql/9.5/main -c config_file=/etc/postgresql/9.5/main/postgresql.conf


Nit: This command is very long. Why not make it shorter by using user = postgres like in the supervisor config example you found?

I tried it the way you suggest, it didn't work, I don't have much time digging up why it didn't work, so meh 🤷‍. Happy to review a follow-up change to this if you're in the mood of making your PR thereafter.

jankeromnes · 2018-01-17T00:11:47Z

support/docker/dev/Dockerfile

+    sudo rm /tmp/supervisord-extra.conf
+
+EXPOSE 3000
+EXPOSE 9000


Nit: You can expose multiple ports in one instruction like so.

jankeromnes · 2018-01-17T00:26:22Z

scripts/watch/client.sh

@@ -2,4 +2,4 @@

 cd client || exit -1

-npm run ng -- server --hmr --host localhost --port 3000
+npm run ng -- server --hmr --host 0.0.0.0 --port 3000


This looks a bit scary, because you're exposing something to the world that was limited to localhost before. Could you please explain why this is needed? Is this a development server?

It's not scary, it's only in development mode: note that this just means exposing the dev web server on the Docker network interface, which is needed if we want to expose the service outside of the container. Keeping it this way (the risk seems nonexistent to me).

Chocobozzz · 2018-01-17T07:23:33Z

support/docker/dev/Dockerfile

@@ -15,10 +15,22 @@ WORKDIR /home/user/PeerTube
 # Configure Cloud9 IDE to use PeerTube's source directory as workspace (-w).
 RUN sudo sed -i "s/-w \/home\/user/-w \/home\/user\/PeerTube/" /etc/supervisord.conf

+# Install dependencies.
+RUN yarn install


RUN yarn install --pure-lockfile

bnjbvr · 2018-01-25T09:57:34Z

Thanks for the reviews, @Chocobozzz @jankeromnes ! Mind to have another look, please? (this works on my machine; since the basic configuration works, and we still have no guarantees that absolute links will work in Janitor anyways, I'd like to not invest too much time on this)

Chocobozzz · 2018-01-25T10:06:54Z

Seems good for me. Waiting @jankeromnes validation.

Even if it does not work yet, we can merge and fix it in another PR :)

jankeromnes

Looks good to me, thanks a lot for the great work! 🎉 🥇 💯

I can't wait to try this out in Janitor! 😄

PeerTube pre-build was removed in Chocobozzz#207 but is still valuable.

PeerTube pre-build was removed in #207 but is still valuable.

bnjbvr force-pushed the janitor branch from 5c9e26a to e9b0263 Compare January 16, 2018 23:06

bnjbvr force-pushed the janitor branch 3 times, most recently from feea98e to 7432554 Compare January 16, 2018 23:49

bnjbvr changed the title ~~WIP: Fix Janitor configuration;~~ Fix Janitor configuration; Jan 16, 2018

bnjbvr force-pushed the janitor branch from 7432554 to 6315b86 Compare January 16, 2018 23:54

jankeromnes approved these changes Jan 17, 2018

View reviewed changes

Chocobozzz reviewed Jan 17, 2018

View reviewed changes

Fixes Chocobozzz#205: Fix Janitor configuration;

09c1c36

bnjbvr force-pushed the janitor branch from 6315b86 to 09c1c36 Compare January 25, 2018 09:54

jankeromnes approved these changes Jan 25, 2018

View reviewed changes

Chocobozzz merged commit a585af3 into Chocobozzz:develop Jan 25, 2018

jankeromnes added a commit to jankeromnes/PeerTube that referenced this pull request Jan 25, 2018

Pre-build PeerTube again in dev Dockerfile.

da7b93a

PeerTube pre-build was removed in Chocobozzz#207 but is still valuable.

jankeromnes mentioned this pull request Jan 25, 2018

Pre-build PeerTube again in dev Dockerfile. #251

Merged

Chocobozzz pushed a commit that referenced this pull request Jan 25, 2018

Pre-build PeerTube again in dev Dockerfile.

897cab4

PeerTube pre-build was removed in #207 but is still valuable.

bnjbvr deleted the janitor branch February 13, 2018 18:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Janitor configuration; #207

Fix Janitor configuration; #207

bnjbvr commented Jan 16, 2018 •

edited

Loading

jankeromnes commented Jan 16, 2018

bnjbvr commented Jan 16, 2018

bnjbvr commented Jan 16, 2018

jankeromnes left a comment

jankeromnes Jan 17, 2018

Chocobozzz Jan 17, 2018

bnjbvr Jan 25, 2018

jankeromnes Jan 17, 2018

bnjbvr Jan 25, 2018

jankeromnes Jan 17, 2018

jankeromnes Jan 17, 2018

bnjbvr Jan 25, 2018

jankeromnes Jan 17, 2018

Chocobozzz Jan 17, 2018 •

edited

Loading

jankeromnes Jan 17, 2018

bnjbvr Jan 25, 2018

jankeromnes Jan 17, 2018

jankeromnes Jan 17, 2018

bnjbvr Jan 25, 2018

Chocobozzz Jan 17, 2018

bnjbvr commented Jan 25, 2018

Chocobozzz commented Jan 25, 2018

jankeromnes left a comment

Fix Janitor configuration; #207

Fix Janitor configuration; #207

Conversation

bnjbvr commented Jan 16, 2018 • edited Loading

jankeromnes commented Jan 16, 2018

bnjbvr commented Jan 16, 2018

bnjbvr commented Jan 16, 2018

jankeromnes left a 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

Chocobozzz Jan 17, 2018 • edited Loading

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

bnjbvr commented Jan 25, 2018

Chocobozzz commented Jan 25, 2018

jankeromnes left a comment

Choose a reason for hiding this comment

bnjbvr commented Jan 16, 2018 •

edited

Loading

Chocobozzz Jan 17, 2018 •

edited

Loading