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.

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

Dockerfile.dev having more build layers #65

Merged
merged 1 commit into from
Jul 8, 2017
Merged

Conversation

vadmeste
Copy link
Member

@vadmeste vadmeste commented Jul 1, 2017

Building an image is slow, this change adds incremental build feature

This PR is minimal so it will be easy to discuss it

@vadmeste vadmeste changed the title [WIP] Dockerfile.dev having more build layers Dockerfile.dev having more build layers Jul 2, 2017
@nitisht
Copy link
Contributor

nitisht commented Jul 3, 2017

This PR is minimal so it will be easy to discuss it

@vadmeste there are several file changes, as per the proposal #59 it felt you planned to change only the Dockerfile. Could please explain what is the advantage in doing this..

@nitisht nitisht added the blocked label Jul 3, 2017
@nitisht
Copy link
Contributor

nitisht commented Jul 3, 2017

blocked till further discussion

@vadmeste
Copy link
Member Author

vadmeste commented Jul 5, 2017

@vadmeste there are several file changes, as per the proposal #59 it felt you planned to change only the Dockerfile. Could please explain what is the advantage in doing this..

Building the image takes so much time for me (11 minutes), I guess the same thing for you. This PR will just take one docker feature (incremental build) so we can reduce image building time when we change a code in the tests. For me, this PR reduces building image to 3 minutes and we can do more.

@harshavardhana
Copy link
Member

Now building image is automated from Travis and saved on our docker registry is this activity still needed ?

@deekoder deekoder removed the blocked label Jul 5, 2017
@vadmeste
Copy link
Member Author

vadmeste commented Jul 5, 2017

Now building image is automated from Travis and saved on our docker registry is this activity still needed ?

I think this is still good to have to fully test locally before creating a PR.

@harshavardhana
Copy link
Member

Let's keep it blocked for now for further discussion.

@nitisht nitisht added the blocked label Jul 5, 2017
@krishnasrinivas
Copy link

krishnasrinivas commented Jul 6, 2017

Ideally, if I am making changes to mint/run/core/minio-js/test/functional-tests.js this is how I would want to test it before making the PR:

  • docker run IMAGE -v ~/dev/mint:/mint -it /bin/bash
    This will give me a bash shell in a container
  • cd /mint/run/core/minio-js
  • npm test

This should test my changes. IMAGE would already be built using Dockerfile.dev
i.e to test my changes locally I should not even build a docker image everytime but just use the image built by Dockerfile.dev

@vadmeste
Copy link
Member Author

vadmeste commented Jul 6, 2017

@krishnasrinivas, I agree since we also have Travis which tests Dockerfile (and not Dockerfile.dev)

@harshavardhana
Copy link
Member

Ideally, if I am making changes to mint/run/core/minio-js/test/functional-tests.js this is how I would want to test it before making the PR:

docker run IMAGE -v ~/dev/mint:/mint -it /bin/bash
This will give me a bash shell in a container
cd /mint/run/core/minio-js
npm test
This should test my changes. IMAGE would already be built using Dockerfile.dev
i.e to test my changes locally I should not even build a docker image everytime but just use the image built by Dockerfile.dev

All functional tests belong to SDKs they are not built with mint or have no awareness other than look for MINT_DATA_DIR. If it works on your laptop it should work fine on mint too. First step towards removing custom library code from mint is already under way. So it means that we will never keep any custom functional test copy of SDKs in mint.

We should follow this with all libraries. Mint only picks from the released tag and builds those functional tests only. So the scenario what you are talking about is not going to happen and shouldn't happen ideally. Since the only test the functional test are doing is to check for MINT_DATA_DIR and read the files in the data directory. As new releases are made for each library you would send a PR bumping the SDK release version numbers.

The problem here is the build process itself for development, since we install all dependencies all the time it takes a lot of time to build. Even if we purge them at the end if we split the layer it will facilitate this.

@krishnasrinivas
Copy link

Ideally, if I am making changes to mint/run/core/minio-js/test/functional-tests.js this is how I would want to test it before making the PR:

All functional tests belong to SDKs they are not built with mint or have no awareness other than look for MINT_DATA_DIR. If it works on your laptop it should work fine on mint too. First step towards removing custom library code from mint is already under way. So it means that we will never keep any custom functional test copy of SDKs in mint.

I was taking SDK functional tests as an example. Even if the client-SDK's functional tests are not included in the mint repo the problem applies to non SDK functional tests that will part of mint repo.

The underlying point in dev->test->dev->test->dev->test cycle of mint, the developer productivity gets affected if Dockerfile.dev takes a lot of time to build and run.

@harshavardhana
Copy link
Member

The underlying point in dev->test->dev->test->dev->test cycle of mint, the developer productivity gets affected if Dockerfile.dev takes a lot of time to build and run.

This shouldn't be done inside mint is what i am saying. There is nothing special mint provides which you cannot have on your Desktop. Develop it outside of mint first and then include it. Building image is when you have everything ready to be finally tested, don't write code on the mint itself.

It takes 10mins to build the docker image what is the total reduction time required for this to be an acceptable number? apart from this you can pull and login to the travis built container as well which can be used like a regular VM style and copy the changes out using docker cp

@krishnasrinivas
Copy link

This shouldn't be done inside mint is what i am saying. There is nothing special mint provides which you cannot have on your Desktop.

I see, I misunderstood the purpose of Dockerfile.dev then.

@vadmeste
Copy link
Member Author

vadmeste commented Jul 7, 2017

We will avoid the complicated pre.sh, post.sh and install.sh, @harshavardhana proposed a simple change which keeps the current project tree (hence simplicity) and allows developers to easily gain image build time by reordering sdks build commands inside Docker.dev.

@nitisht nitisht removed the blocked label Jul 7, 2017
@vadmeste
Copy link
Member Author

vadmeste commented Jul 7, 2017

So this is ready for review again

@nitisht
Copy link
Contributor

nitisht commented Jul 7, 2017

LGTM, tested. Gets the work done.

One comment @vadmeste , could you please update the README with details on how to use this effectively, for example how to change the Dokcerfile if someone edits the aws-sdk-php so that it builds fast.

Building an image is slow, this change helps developers to reduce image
build time by making it easier to build only one sdk or reorder sdks
building to avoid affecting the incremental build.
@vadmeste
Copy link
Member Author

vadmeste commented Jul 7, 2017

One comment @vadmeste , could you please update the README with details on how to use this effectively

@nitisht, I added a short text which explains how to customize Dockerfile.dev for faster build, what do you think ?

@nitisht
Copy link
Contributor

nitisht commented Jul 7, 2017

@nitisht, I added a short text which explains how to customize Dockerfile.dev for faster build, what do you think ?

That should be enough @vadmeste . LGTM

@nitisht
Copy link
Contributor

nitisht commented Jul 7, 2017

Any review here @harshavardhana @krishnasrinivas @krisis

@krishnasrinivas
Copy link

@nitisht, I added a short text which explains how to customize Dockerfile.dev for faster build, what do you think ?

👍 indicates that it need not be used as is.

@nitisht nitisht merged commit 8f87902 into minio:master Jul 8, 2017
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.

5 participants