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.

Sign up for GitHub

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

Fix Janitor configuration; #207

Merged
merged 1 commit into from
Jan 25, 2018
Merged

Fix Janitor configuration; #207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion scripts/watch/client.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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).

13 changes: 11 additions & 2 deletions support/docker/dev/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,19 @@ 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 --pure-lockfile

# 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

npm run dev will build the client on the fly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What @Chocobozzz said. Closing.

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

ADD supervisord.conf /tmp/supervisord-extra.conf
RUN cat /tmp/supervisord-extra.conf | sudo tee -a /etc/supervisord.conf

EXPOSE 3000 9000
3 changes: 3 additions & 0 deletions support/docker/dev/create_user.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
create database peertube_dev;
create user peertube password 'peertube';
grant all privileges on database peertube_dev to peertube;
12 changes: 8 additions & 4 deletions support/docker/dev/janitor.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@
"label": "SSH",
"proxy": "none"
},
"3000": {
"label": "PeerTube web app",
"proxy": "https",
"preview": true
},
"8088": {
"label": "VNC",
"proxy": "https"
Expand All @@ -19,13 +24,12 @@
"proxy": "https"
},
"9000": {
"label": "PeerTube",
"proxy": "https",
"preview": true
"label": "PeerTube API",
"proxy": "https"
}
},
"scripts": {
"Start PeerTube": "npm start",
"Start PeerTube": "npm run dev",
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Owner

@Chocobozzz Chocobozzz Jan 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

"Build PeerTube": "npm run build",
"Run tests": "npm test",
"Update source code": "git pull --rebase origin",
Expand Down
3 changes: 3 additions & 0 deletions support/docker/dev/supervisord.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.