-
Notifications
You must be signed in to change notification settings - Fork 241
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.
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
Feature/zenko 485 eve on s3 #1278
Conversation
Hello ssalaues,My role is to assist you with the merge of this Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
5212df1
to
bca4bef
Compare
Do it again human slave!:point_right: :runner: (Oh and the pull request has been updated, by the way.)
PR has been updated. Reviewers, please be cautious. |
bca4bef
to
ed90015
Compare
PR has been updated. Reviewers, please be cautious. |
Fixed commit messages |
ed90015
to
3d29160
Compare
PR has been updated. Reviewers, please be cautious. |
Fixed post merge step |
example of the post-merge docker build step |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick review. LGTM in general.
- '/home/eve/workspace' | ||
steps: | ||
- Git: *clone | ||
- ShellCommand: *npm-install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we use yarn? It's faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try it out. Should work
eve/workers/build/Dockerfile
Outdated
COPY ./s3_packages.list ./buildbot_worker_packages.list /tmp/ | ||
RUN apt-get update \ | ||
&& cat /tmp/*packages.list | xargs apt-get install -y \ | ||
&& wget https://nodejs.org/dist/v6.9.5/node-v6.9.5-linux-x64.tar.xz \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we try using n
to manage node.js distribution? This way we can automatically install n 6.x
and get the latest minor-minor version.
https://github.com/tj/n
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's a good idea. I'll add that in.
@bert-e wait |
@@ -4,7 +4,7 @@ MAINTAINER Giorgio Regni <gr@scality.com> | |||
WORKDIR /usr/src/app | |||
|
|||
# Keep the .git directory in order to properly report version | |||
COPY . /usr/src/app | |||
COPY ./package.json . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. just curious as to why only the package.json
is copied here rather than the whole dir
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is where the magic of docker layers becomes useful. So say you are changing code and need to rebuild your docker image to test but you dependencies stay the same. If this was built previously, then on consecutive runs Docker will skip this step along with the following RUN step where all the system packages and npm packages are installed and use the cached layers from the previous builds.
It then will use the step at line 18 to copy the remaining code base into the image which wont be cached because of code changes.
3d29160
to
211b4b7
Compare
Do it again human slave!:point_right: :runner: (Oh and the pull request has been updated, by the way.)
PR has been updated. Reviewers, please be cautious. |
|
211b4b7
to
728f1b4
Compare
Do it again human slave!:point_right: :runner: (Oh and the pull request has been updated, by the way.)
PR has been updated. Reviewers, please be cautious. |
Allows faster CI and dev builds of the Docker image through the use of cached layers.
728f1b4
to
387cac9
Compare
PR has been updated. Reviewers, please be cautious. |
Merging manually due to a know Azure issue in CI. |
Adds basic Eve functionality using both kube and docker workers that can be the template for future additions.
2nd commit modifies the Dockerfile slightly to improve CI and local build time