Skip to content
This repository was archived by the owner on Jul 3, 2023. It is now read-only.

Conversation

@aimbot31
Copy link
Member

@aimbot31 aimbot31 commented Jul 31, 2019

Added the Dockerfile for CloudKitty api and its dependencies.
Also added the Dockerfile for CloudKitty processor
and updated the README to include the Dockerfiles documentation.

@aimbot31 aimbot31 requested a review from lukapeschke July 31, 2019 15:17
@aimbot31 aimbot31 force-pushed the dockerfiles-cloudkitty branch 3 times, most recently from b123706 to f17fdf4 Compare August 1, 2019 07:58
@aimbot31
Copy link
Member Author

aimbot31 commented Aug 1, 2019

@lukapeschke i think it should be good now

Copy link
Contributor

@lukapeschke lukapeschke left a comment

Choose a reason for hiding this comment

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

A few remarks on the Dockerfiles and the README, see inline comments

README.md Outdated

### api

Contains the Dockerfiles for the api side of CloudKitty and its dependencies.
Copy link
Contributor

Choose a reason for hiding this comment

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

"the Dockerfile", no S. "For CloudKitty's API": api side sounds a bit french 😉

README.md Outdated

### processor

Contains the Dockerfiles for the processor side of CloudKitty.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same remarks a for the API.


RUN pip install cloudkitty

RUN apt-get purge -y \
Copy link
Contributor

Choose a reason for hiding this comment

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

Please merge the three RUN instructions above into one.


COPY apache_cloudkitty.conf /etc/apache2/sites-available/cloudkitty.conf

RUN a2ensite cloudkitty
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm maybe this could be done by directly copying the cloudkitty.conf file to sites-enabled ?


RUN a2ensite cloudkitty

ADD run_apache.sh /root/run_apache.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a COPY

Added the Dockerfile for CloudKitty api and its dependencies.
Also added the Dockerfile for CloudKitty processor
and updated the README to include the Dockerfiles documentation.
@aimbot31 aimbot31 force-pushed the dockerfiles-cloudkitty branch from f17fdf4 to 25aeeb9 Compare August 1, 2019 08:33
Copy link
Contributor

@lukapeschke lukapeschke left a comment

Choose a reason for hiding this comment

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

LGTM

@lukapeschke lukapeschke merged commit 4f12574 into master Aug 1, 2019
@lukapeschke lukapeschke deleted the dockerfiles-cloudkitty branch August 1, 2019 08:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants