Allow re-use of Docker image (reopen #319) #341

130s · 2018-12-12T23:25:52Z

What does this PR change

Re-opening #319 using the commits from the requester there.
This adds a feature to run docker commit from industrial_ci, so that the users can access the built docker image for later usage.

Review comments up to this point #319 (review) are hopefully addressed.

Maturity of the suggested feature

At the time of opening this PR, the feature is experimental. We've been actively using on our (private) repos, and I found there are still some basic issues that need to be fixed in order for this feature to be integrated into a full-automated cycle. That said, we've been able to use this feature in semi-automated situation where small manual change to the built image makes the image usable.

Instead of trying to fix all of those issues in a single initial PR, I'd suggest to move on, claiming experimental, merge, open tickets, and keep iterating.

Review items

@130s Locally test.
CI pass.
Review changes.

130s · 2018-12-12T23:48:25Z

Not sure why Travis CI didn't grab this. Close-reopening.

doc/index.rst

mathias-luedtke

Code is ready for merge.

Please fix the travis config
Update documentation (from Expose COMMIT_IMAGE to user #319)

.travis.yml

…and variables.

130s · 2018-12-23T11:23:45Z

@ipa-mdl Thank you for the review #341 (review). Hopefully updated commit addressed all comments.

mathias-luedtke

Code and documentation are alright (just a minor glitch left).
But please update rerun_ci accordingly.

industrial_ci/src/docker.sh

Addressing https://github.com/ros-industrial/industrial_ci/pull/319/files#r220267620 [feat:commit_img] Add a section to readme. [feat:commit_img] Remove 'COMMIT_IMAGE' variable. Add a test. Address https://github.com/ros-industrial/industrial_ci/pull/319/files#r222089723 [feat:commit_img] Adding a test for DOCKER_COMMIT_IMAGE. [feat:commit_img] Simplify 'docker commit' variable names. Revert change in rerun_ci (address a review commnet ros-industrial#319 (review)). Rename the variables to clarify they are related to Docker. Addressing https://github.com/ros-industrial/industrial_ci/pull/319/files#r220267620 expose COMMIT_IMAGE variable to user. Updates related documentation and variables. [feat:commit_img] Remove 'COMMIT_IMAGE' variable. Add a test. Address https://github.com/ros-industrial/industrial_ci/pull/319/files#r222089723 [feat:commit_img] Simplify 'docker commit' variable names. Revert change in rerun_ci (address a review commnet ros-industrial#319 (review)). [doc] Remove unused, obsolete var name (address https://github.com/ros-industrial/industrial_ci/pull/319/files#r230591444) [doc] Remove unused var (address https://github.com/ros-industrial/industrial_ci/pull/319/files#r230591088). [feat:commit_img] Simplified test (attempting to address https://github.com/ros-industrial/industrial_ci/pull/319/files#r230591026) [feat:commit_img] Simplify test logic (based on suggestion AustinDeric#3 (comment)). [feat:commit_img] Removed obsolete logic (address ros-industrial#319 (comment)). [feat:commit_img] address review comments (ros-industrial#319 (review)) [docker.sh] Indentation (ros-industrial#341 (comment)).

130s · 2018-12-30T15:35:19Z

Commit updated.

Locally I tested rerun_ci, which seems to work fine without DOCKER_COMMIT defined (i.e. I see a temporary Docker image commited), but with the arg the behavior is still the same as no arg.

$ SRCDIR=/tmp/cws_dr/src/ && mkdir -p $SRCDIR && cd $SRCDIR && git clone https://github.com/ros-drivers/openni2_camera
$ cd /tmp && git clone https://github.com/130s/industrial_ci -b feature/docker_commit
$ /tmp/industrial_ci/industrial_ci/scripts/rerun_ci . ROS_DISTRO=melodic ROS_REPO=ros-shadow-fixed
$ /tmp/industrial_ci/industrial_ci/scripts/rerun_ci . ROS_DISTRO=melodic ROS_REPO=ros-shadow-fixed DOCKER_COMMIT="test_rerun_ci" DOCKER_COMMIT_MSG="testing_rerun_msg"

$ docker images| grep -i openni2                                                                                                                                                                                                                                                                                                          
REPOSITORY                                                          TAG                                                           IMAGE ID            CREATED             SIZE
industrial-ci/rerun_ci/openni2_camera                               66d52ad29be5                                                  efa6e1eb336e        2 hours ago         1.76GB

UPDATE: Figured it out the reason why the specified image isn't committed #341 (comment). rerun_ci looks for environment variables while I only specified the command args. Will update.

130s · 2018-12-30T16:00:19Z

Please re-review @ipa-mdl.

mathias-luedtke

Locally I tested rerun_ci, which seems to work fine without DOCKER_COMMIT defined (i.e. I see a temporary Docker image commited), but with the arg the behavior is still the same as no arg.

Yes, this is how rerun_ci works, it commits the image and run further tests in the new images.
That's why I implemented _DOCKER_COMMIT in the first place.

mathias-luedtke · 2018-12-31T01:20:06Z

industrial_ci/scripts/rerun_ci

-_COMMIT_IMAGE_MSG="$repo_dir $*"
-env_hash=($(sha256sum <<< "$_COMMIT_IMAGE_MSG"))
-_COMMIT_IMAGE="industrial-ci/rerun_ci/$(basename "$repo_dir"):${env_hash:0:12}"
+if [ -z $DOCKER_COMMIT_MSG ]; then DOCKER_COMMIT_MSG="$repo_dir $*"; fi


DOCKER_COMMIT_MSG and DOCKER_COMMIT should not get overwritten from the outside, they are crucial to the argument hashing.
Please remove these if clauses.

130s · 2018-12-31T19:27:20Z

TLDR; @ipa-mdl Could you elaborate how you want us to update rerun_ci (mentioned at #341 (review))?

Locally I tested rerun_ci, which seems to work fine without DOCKER_COMMIT defined (i.e. I see a temporary Docker image commited), but with the arg the behavior is still the same as no arg.

Yes, this is how rerun_ci works, it commits the image and run further tests in the new images.
That's why I implemented _DOCKER_COMMIT in the first place.

In this PR DOCKER_COMMIT variable is exposed to users, allowing them to specify the name to save docker image with. Then I thought we'd want rerun_ci script to behave the same. When a user defines the docker image name by DOCKER_COMMIT, wouldn't rerun_ci script save the state into the given image name instead of automatically generated hash value? And if that's correct wouldn't it be ok?

mathias-luedtke · 2019-01-01T17:50:55Z

Could you elaborate how you want us to update rerun_ci

Just remove the if clauses and overwrite DOCKER_COMMIT and DOCKER_COMMIT_MSG (as before).

then I thought we'd want rerun_ci script to behave the same. When a user defines the docker image name by DOCKER_COMMIT, wouldn't rerun_ci script save the state into the given image name instead of automatically generated hash value? And if that's correct wouldn't it be ok?

No.
rerun_ci is just a little helper to take care of DOCKER_COMMITetc.
The hashing is the main functionality. If you change the image name or the message, it does not work properly anymore.
If hashing is not needed, just use run_ci.

130s · 2019-01-02T01:40:56Z

Just remove the if clauses and overwrite DOCKER_COMMIT and DOCKER_COMMIT_MSG (as before).

Ok.

In that case I don't see a point in updating the variable names (_COMMIT_IMAGE to DOCKER_COMMIT etc.) in rerun_ci. I've kept the change but let me know if I should revert.

Also updated the document, as I think the new feature added in this PR doesn't work for rerun (I tested it locally. Not sure if that's what's supposed to be though).

mathias-luedtke · 2019-01-02T11:34:38Z

Thanks!

In that case I don't see a point in updating the variable names (_COMMIT_IMAGE to DOCKER_COMMIT etc.) in rerun_ci. I've kept the change but let me know if I should revert.

It is crucial to rename the variables everywhere. rerun_ci would not work anymore, it built around this feature.

Also updated the document, as I think the new feature added in this PR doesn't work for rerun (I tested it locally. Not sure if that's what's supposed to be though).

I don't see the need for an "exception", but we could add a note that rerun_ci manages these variables for its special purpose.
This PR does not add a new feature, it just exposes an exisiting feature.

I did not make it public in the first place, because the resulting images are not well-formed.
However, I see 2 legit usecases:

rerun_ci
speedup different [build stages](https://github.com/ros-industrial/industrial_ci/pull/286#issuecomment-395332995] (together with Add DOCKER_PULL option #283)

It could even be pushed to a registry and reused as DOCKER_IMAGE, but I would not recommend it.

doc/index.rst

130s · 2019-01-02T19:47:01Z

Updated to address your comment @ipa-mdl.

It could even be pushed to a registry and reused as DOCKER_IMAGE, but I would not recommend it.

Ha, my team has been happilly doing so for a few months :P Indeed built Docker images currently need some manual update (which I'll open tickets about once this PR finishes) but overall being able to automate Docker build as part of normal CI pipeline w/o maintaining Dockerfiles etc. is very helpful for our usecase.

mathias-luedtke · 2019-01-02T20:50:58Z

Ha, my team has been happilly doing so for a few months :P Indeed built Docker images currently need some manual update

That's great to hear :)
But I still do not recommend it to the average user on Travis CI ;)

mathias-luedtke

👍

130s requested a review from mathias-luedtke December 12, 2018 23:25

130s closed this Dec 12, 2018

130s reopened this Dec 12, 2018

mathias-luedtke reviewed Dec 17, 2018

View reviewed changes

doc/index.rst Outdated Show resolved Hide resolved

mathias-luedtke requested changes Dec 17, 2018

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

expose COMMIT_IMAGE variable to user. Updates related documentation …

b8b247e

…and variables.

130s force-pushed the feature/docker_commit branch from 29e0364 to e1d9873 Compare December 23, 2018 11:16

mathias-luedtke requested changes Dec 24, 2018

View reviewed changes

industrial_ci/src/docker.sh Outdated Show resolved Hide resolved

130s mentioned this pull request Dec 30, 2018

New feature release (0.7.0 or higher) #344

Closed

4 tasks

130s force-pushed the feature/docker_commit branch from e1d9873 to 0ac50ba Compare December 30, 2018 13:00

130s added 2 commits December 30, 2018 05:26

[travis.yml] Fix yaml format.

ef50524

130s force-pushed the feature/docker_commit branch from 0ac50ba to 277a7c7 Compare December 30, 2018 15:26

130s force-pushed the feature/docker_commit branch from 277a7c7 to 4b96fc0 Compare December 30, 2018 16:00

mathias-luedtke requested changes Dec 31, 2018

View reviewed changes

[rerun.sh] Rename some variables to adjust to docker commit feature.

289a089

130s force-pushed the feature/docker_commit branch from 4b96fc0 to 58d9577 Compare January 2, 2019 01:35

mathias-luedtke reviewed Jan 2, 2019

View reviewed changes

doc/index.rst Outdated Show resolved Hide resolved

130s force-pushed the feature/docker_commit branch 3 times, most recently from 566c4ef to 7963ecd Compare January 2, 2019 19:42

[doc][rerun.sh] Add some exception.

892c4e9

130s force-pushed the feature/docker_commit branch from 7963ecd to 892c4e9 Compare January 2, 2019 19:44

mathias-luedtke force-pushed the feature/docker_commit branch from aaeee7e to 892c4e9 Compare January 2, 2019 22:39

mathias-luedtke approved these changes Jan 2, 2019

View reviewed changes

mathias-luedtke merged commit 6c97518 into ros-industrial:master Jan 2, 2019

mathias-luedtke mentioned this pull request Jan 2, 2019

Feature: cache images #286

Closed

130s deleted the feature/docker_commit branch January 3, 2019 02:52

130s changed the title ~~Add a feature to commit Docker image (reopen #319)~~ Allow re-use of Docker image (reopen #319) Jan 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow re-use of Docker image (reopen #319) #341

Allow re-use of Docker image (reopen #319) #341

130s commented Dec 12, 2018 •

edited

Loading

130s commented Dec 12, 2018

mathias-luedtke left a comment

130s commented Dec 23, 2018

mathias-luedtke left a comment

130s commented Dec 30, 2018 •

edited

Loading

130s commented Dec 30, 2018

mathias-luedtke left a comment

mathias-luedtke Dec 31, 2018

130s commented Dec 31, 2018

mathias-luedtke commented Jan 1, 2019

130s commented Jan 2, 2019

mathias-luedtke commented Jan 2, 2019

130s commented Jan 2, 2019

mathias-luedtke commented Jan 2, 2019

mathias-luedtke left a comment

Allow re-use of Docker image (reopen #319) #341

Allow re-use of Docker image (reopen #319) #341

Conversation

130s commented Dec 12, 2018 • edited Loading

What does this PR change

Maturity of the suggested feature

Review items

130s commented Dec 12, 2018

mathias-luedtke left a comment

Choose a reason for hiding this comment

130s commented Dec 23, 2018

mathias-luedtke left a comment

Choose a reason for hiding this comment

130s commented Dec 30, 2018 • edited Loading

130s commented Dec 30, 2018

mathias-luedtke left a comment

Choose a reason for hiding this comment

mathias-luedtke Dec 31, 2018

Choose a reason for hiding this comment

130s commented Dec 31, 2018

mathias-luedtke commented Jan 1, 2019

130s commented Jan 2, 2019

mathias-luedtke commented Jan 2, 2019

130s commented Jan 2, 2019

mathias-luedtke commented Jan 2, 2019

mathias-luedtke left a comment

Choose a reason for hiding this comment

130s commented Dec 12, 2018 •

edited

Loading

130s commented Dec 30, 2018 •

edited

Loading