Skip to content

Dev cleanup#12

Open
yelidrissi wants to merge 7 commits intodergachev:masterfrom
yelidrissi:dev-cleanup
Open

Dev cleanup#12
yelidrissi wants to merge 7 commits intodergachev:masterfrom
yelidrissi:dev-cleanup

Conversation

@yelidrissi
Copy link
Contributor

No description provided.

robert-ngo and others added 2 commits July 18, 2018 17:04
…ionally equivalent to the older Node.js server.

Many changes were made across the project in order to achieve this.
Copy link
Owner

@dergachev dergachev left a comment

Choose a reason for hiding this comment

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

Here's my review

.env Outdated
@@ -0,0 +1,4 @@
PORT_PREFIX=80
DOCKER_IMAGE=robert_gdocs
Copy link
Owner

Choose a reason for hiding this comment

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

Fix prefix, perhaps gdocs_server

Dockerfile Outdated
@@ -1,19 +1,7 @@
FROM ubuntu:trusty
MAINTAINER Alex Dergachev <alex@evolvingweb.ca>
FROM dergachev/gdocs-export:latest
Copy link
Owner

Choose a reason for hiding this comment

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

you are killing my Dockerfile that builds dergachev/gdocs-export image. Problematic.

COPY server/envvars /etc/apache2/envvars
COPY server/envvars /etc/apache2/envvars.bak
RUN cat /etc/apache2/envvars.bak | tr -s '\r' '\n' > /etc/apache2/envvars
RUN ["/bin/bash","-c","source /etc/apache2/envvars"]
Copy link
Owner

Choose a reason for hiding this comment

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

@robert please verify necessity

Dockerfile Outdated



RUN service apache2 restart
Copy link
Owner

Choose a reason for hiding this comment

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

quite sure not necessary

EXPOSE 12736
WORKDIR /var/gdocs-export/

ARG extra
Copy link
Owner

Choose a reason for hiding this comment

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

@robert please confirm this makes sense

@@ -0,0 +1,120 @@
#!/usr/bin/env python
Copy link
Owner

Choose a reason for hiding this comment

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

Remove this .bak file, it's redundant

@@ -1,43 +1,31 @@
#!/usr/bin/env python
Copy link
Owner

Choose a reason for hiding this comment

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

separate PR please for this change

@@ -0,0 +1,47 @@
# envvars - default environment variables for apache2ctl
Copy link
Owner

Choose a reason for hiding this comment

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

remove this file

@@ -9,6 +9,81 @@
include('includes/Message.php');
Copy link
Owner

Choose a reason for hiding this comment

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

get rid of Message.php if it's not used

}

function upload() {
if(!isset($_REQUEST['id']) || !isset($_REQUEST['token'])) endit(400, "Bad Request.");
Copy link
Owner

Choose a reason for hiding this comment

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

run through drupal code sniffer (drupalcs)

yelidrissi and others added 3 commits July 25, 2018 15:32
@yelidrissi
Copy link
Contributor Author

The tasks assigned during the review process have been dealt with.

CMD apachectl -D FOREGROUND

# allow gdocs to run script as root
RUN printf "\ngdocs ALL=(root) NOPASSWD: /var/gdocs-export/server/scripts/web-convert-gdoc.sh\n" >> /etc/sudoers

Choose a reason for hiding this comment

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

Should remove this line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants