Implement a basic, first pass `odo push` with Docker support #2806

johnmcollier · 2020-04-02T22:48:51Z

What type of PR is this?
/kind feature

What does does this PR do / why we need it:
This PR implements a basic, first pass implementation of odo push with local Docker containers when the PushTarget preference is set to Docker. Currently it's limited to just deploying the containers and setting up the shared source volume. This will unblock folks who are assigned issues that are dependent on this functionality (such as #2717, #2580, #2581)

So, what does the PR do in all?

Creates a Docker platform adapter for interacting with devfile components in Docker
- The Docker adapter is used only if experimental is true and PushTarget is set to Docker
Creates a Docker component adapter, which does the following
- Push pulls images and starts containers based on what was specified in the devfile. Each container is labeled with component=<odo-componentName> and alias=<devfile-component-alias> that provide an easy way to group and find the pods associated with an odo component
- DoesComponentExist returns true if there's at least one container running that's labeled with component=<odo-componentName>
Unit tests
Integration tests

Which issue(s) this PR fixes:

Fixes #2579

How to test changes / Special notes to the reviewer:

Have Docker installed and running on your system
Build my branch
Set both experimental mode to true and PushTarget to Docker
- odo preference set experimental true
- odo preference set pushtarget docker
Create a devfile component
- odo create openLiberty
odo push

Signed-off-by: John Collier <John.J.Collier@ibm.com>

amitkrout · 2020-04-05T08:49:23Z

Integration tests coming in a later commit to this branch. Needed to get this PR up first before experimenting with Travis

👍
@johnmcollier It won't be much complex to run the test on travis. As we have oc cluster up configured to run for each job on travis which uses default running docker instances as a pre requisite. So you just need to write the test scenario, write the make target for it then call it in .travis.yaml.

As part of running the same test on 4.* is concerned there we need to figure out how to do it (Possibly we need to configure docker instances in the test container). I would suggest create a separate issue for running the same test against 4.* cluster and assign it to @prietyc123.

Signed-off-by: John Collier <John.J.Collier@ibm.com>

johnmcollier · 2020-04-06T19:23:43Z

Updated the PR with integration tests 😃

For now they're just running in Travis, I'll open an issue later today to track running them elsewhere.

Signed-off-by: John Collier <John.J.Collier@ibm.com>

codecov · 2020-04-06T20:03:15Z

Codecov Report

Merging #2806 into master will increase coverage by 0.80%.
The diff coverage is 73.70%.

@@            Coverage Diff             @@
##           master    #2806      +/-   ##
==========================================
+ Coverage   43.06%   43.87%   +0.80%     
==========================================
  Files          97      102       +5     
  Lines        8917     9147     +230     
==========================================
+ Hits         3840     4013     +173     
- Misses       4713     4761      +48     
- Partials      364      373       +9

Impacted Files	Coverage Δ
pkg/devfile/adapters/helper.go	`10.71% <0.00%> (-5.96%)`	⬇️
pkg/lclient/client.go	`0.00% <ø> (ø)`
pkg/lclient/containers.go	`58.97% <39.13%> (-29.92%)`	⬇️
pkg/lclient/images.go	`60.00% <60.00%> (ø)`
pkg/devfile/adapters/docker/component/utils.go	`65.51% <65.51%> (ø)`
pkg/devfile/adapters/docker/component/adapter.go	`70.58% <70.58%> (ø)`
pkg/devfile/adapters/docker/utils/utils.go	`100.00% <100.00%> (ø)`
pkg/lclient/fakeclient.go	`66.30% <100.00%> (+23.68%)`	⬆️
pkg/lclient/generators.go	`100.00% <100.00%> (ø)`
pkg/lclient/storage.go	`100.00% <100.00%> (ø)`
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 11e04b6...c3020b8. Read the comment docs.

pkg/devfile/adapters/docker/component/adapter.go

maysunfaisal

Thx for this PR, impressed by the quick turnaround considering all the logic that is in here!

Review is mostly just re-arranging code and functions to be consistent with the devfile kubernetes package, since they seem out of place looking at the two packages simultaneously and few nits here and there which can go either way depending on how you prefer it.

pkg/devfile/adapters/helper.go

pkg/lclient/containers.go

pkg/devfile/adapters/docker/component/adapter.go

pkg/lclient/containers.go

pkg/devfile/adapters/docker/component/adapter.go

Signed-off-by: John Collier <John.J.Collier@ibm.com>

maysunfaisal

Minor changes and questions

Looks good and works well, I can add lgtm after small cleanups

pkg/lclient/storage.go

pkg/devfile/adapters/docker/component/utils.go

maysunfaisal · 2020-04-10T17:11:46Z

pkg/devfile/adapters/docker/component/utils.go

+				// Start the container
+				err = a.startContainer(componentName, projectVolumeName, comp)
+				if err != nil {
+					return errors.Wrapf(err, "Unable to start container for devfile component %s", *comp.Alias)


Same comment about s.End(false)

See my reply to your other comment.

pkg/devfile/adapters/docker/component/utils.go

tests/integration/docker/cmd_docker_devfile_push_test.go

maysunfaisal · 2020-04-10T17:19:20Z

tests/helper/helper_docker.go

+}
+
+// NewDockerRunner initializes new DockerRunner
+func NewDockerRunner(ocPath string) DockerRunner {


ocPath to dockerPath

maysunfaisal · 2020-04-10T17:22:05Z

tests/integration/docker/cmd_docker_devfile_push_test.go

@@ -0,0 +1,81 @@
+package docker


how do you feel about all devfile tests being under the devfile dir? and then separated out to tests/integration/devfile/docker and tests/integration/devfile/kube

Makes sense, fixed. I moved the docker tests to tests/integration/devfile/docker, we can move the kube tests later.

tests/integration/docker/cmd_docker_devfile_push_test.go

Signed-off-by: John Collier <John.J.Collier@ibm.com>

maysunfaisal · 2020-04-10T18:04:43Z

changes look good
/lgtm

Signed-off-by: John Collier <John.J.Collier@ibm.com>

johnmcollier · 2020-04-15T01:24:11Z

Had to fix a merge conflict

maysunfaisal · 2020-04-15T06:17:17Z

adding back lgtm after rebase
/lgtm

maysunfaisal · 2020-04-15T06:17:36Z

/retest

kadel · 2020-04-15T13:03:11Z

/retest
/approve

openshift-ci-robot · 2020-04-15T13:03:34Z

@kadel: The /retest command does not accept any targets.
The following commands are available to trigger jobs:

/test unit
/test v4.2-integration-e2e-benchmark
/test v4.3-integration-e2e-benchmark
/test v4.4-integration-e2e-benchmark

Use /test all to run all jobs.

In response to this:

/retest
/approve

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2020-04-15T13:03:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kadel

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [kadel]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2020-04-15T13:52:54Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-15T14:19:08Z

/retest

Please review the full test history for this PR and help us cut down flakes.

johnmcollier · 2020-04-15T22:30:13Z

/retest

…developer#2806) * Implement docker adapter Signed-off-by: John Collier <John.J.Collier@ibm.com> * Docker component support Signed-off-by: John Collier <John.J.Collier@ibm.com> * Implement odo push for Docker Signed-off-by: John Collier <John.J.Collier@ibm.com> * Create helper file for integration tests Signed-off-by: John Collier <John.J.Collier@ibm.com> * Fix build and test issues Signed-off-by: John Collier <John.J.Collier@ibm.com> * Don't require kube client for odo create Signed-off-by: John Collier <John.J.Collier@ibm.com> * Fix storage tests Signed-off-by: John Collier <John.J.Collier@ibm.com> * Address gosec messages Signed-off-by: John Collier <John.J.Collier@ibm.com> * Finish docker integration tests Signed-off-by: John Collier <John.J.Collier@ibm.com> * Fix borked git rebase Signed-off-by: John Collier <John.J.Collier@ibm.com> * Address review comments Signed-off-by: John Collier <John.J.Collier@ibm.com> * Fix merge conflict issues Signed-off-by: John Collier <John.J.Collier@ibm.com> * Fix more merge conflict issues Signed-off-by: John Collier <John.J.Collier@ibm.com> * Fix more merge conflicts in tests Signed-off-by: John Collier <John.J.Collier@ibm.com> * Address rev Signed-off-by: John Collier <John.J.Collier@ibm.com> * Don't require kube context Signed-off-by: John Collier <John.J.Collier@ibm.com>

kadel · 2020-04-21T15:07:47Z

As part of running the same test on 4.* is concerned there we need to figure out how to do it (Possibly we need to configure docker instances in the test container). I would suggest create a separate issue for running the same test against 4.* cluster and assign it to @prietyc123.

Why we would want to run this against clusters. No cluster is used in this case, it is just using Docker daemon

johnmcollier added 8 commits March 26, 2020 10:26

Implement docker adapter

a7bfdb2

Signed-off-by: John Collier <John.J.Collier@ibm.com>

Docker component support

a5d9576

Signed-off-by: John Collier <John.J.Collier@ibm.com>

Implement odo push for Docker

20280ca

Signed-off-by: John Collier <John.J.Collier@ibm.com>

Create helper file for integration tests

3fff974

Signed-off-by: John Collier <John.J.Collier@ibm.com>

Merge branch 'master' of github.com:openshift/odo into dockeradapter

ae26553

Signed-off-by: John Collier <John.J.Collier@ibm.com>

Fix build and test issues

979a8db

Signed-off-by: John Collier <John.J.Collier@ibm.com>

Don't require kube client for odo create

771338e

Signed-off-by: John Collier <John.J.Collier@ibm.com>

Fix storage tests

8d107d8

Signed-off-by: John Collier <John.J.Collier@ibm.com>

openshift-ci-robot added the kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation label Apr 2, 2020

openshift-ci-robot requested review from mohammedzee1000 and prietyc123 April 2, 2020 22:49

Address gosec messages

aa31e56

Signed-off-by: John Collier <John.J.Collier@ibm.com>

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Apr 5, 2020

johnmcollier added 2 commits April 6, 2020 12:40

Merge branch 'master' of github.com:openshift/odo into dockeradapter

0ee6ddf

Signed-off-by: John Collier <John.J.Collier@ibm.com>

Finish docker integration tests

5e30946

Signed-off-by: John Collier <John.J.Collier@ibm.com>

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Apr 6, 2020

Fix borked git rebase

ac3cda5

Signed-off-by: John Collier <John.J.Collier@ibm.com>

girishramnani reviewed Apr 8, 2020

View reviewed changes

pkg/devfile/adapters/docker/component/adapter.go Outdated Show resolved Hide resolved

girishramnani reviewed Apr 8, 2020

View reviewed changes

pkg/devfile/adapters/docker/component/adapter.go Outdated Show resolved Hide resolved

maysunfaisal suggested changes Apr 8, 2020

View reviewed changes

openshift-ci-robot assigned maysunfaisal Apr 8, 2020

johnmcollier added 5 commits April 8, 2020 17:20

Address review comments

61e75fd

Signed-off-by: John Collier <John.J.Collier@ibm.com>

Merge branch 'master' of github.com:openshift/odo into dockeradapter

eafe40f

Fix merge conflict issues

a21d4b2

Signed-off-by: John Collier <John.J.Collier@ibm.com>

Fix more merge conflict issues

97f1c45

Signed-off-by: John Collier <John.J.Collier@ibm.com>

Fix more merge conflicts in tests

fecfae5

Signed-off-by: John Collier <John.J.Collier@ibm.com>

maysunfaisal reviewed Apr 10, 2020

View reviewed changes

Address rev

9b0a89c

Signed-off-by: John Collier <John.J.Collier@ibm.com>

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. Required by Prow. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. labels Apr 10, 2020

johnmcollier added 2 commits April 14, 2020 21:13

Merge branch 'master' of github.com:openshift/odo into dockeradapter

c9e4c32

Signed-off-by: John Collier <John.J.Collier@ibm.com>

Don't require kube context

c3020b8

Signed-off-by: John Collier <John.J.Collier@ibm.com>

openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. Required by Prow. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. labels Apr 15, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 15, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Apr 15, 2020

openshift-merge-robot merged commit 22a6364 into redhat-developer:master Apr 16, 2020

jgwest mentioned this pull request Apr 16, 2020

Use 'odo delete` to delete a component that's running locally with odo #2905

Merged

kadel mentioned this pull request Apr 16, 2020

Odo push should deploy local Docker containers when pushtarget is set to Docker #2579

Closed

3 tasks

amitkrout mentioned this pull request Apr 18, 2020

Enable docker devfile push test in OpenShift CI #2921

Closed

jgwest mentioned this pull request Apr 25, 2020

Add storage volume deletion to 'odo delete' docker devfile deletion #2993

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement a basic, first pass `odo push` with Docker support #2806

Implement a basic, first pass `odo push` with Docker support #2806

johnmcollier commented Apr 2, 2020 •

edited

amitkrout commented Apr 5, 2020

johnmcollier commented Apr 6, 2020

codecov bot commented Apr 6, 2020 •

edited

maysunfaisal left a comment

maysunfaisal left a comment

maysunfaisal Apr 10, 2020

johnmcollier Apr 10, 2020

maysunfaisal Apr 10, 2020

johnmcollier Apr 10, 2020

maysunfaisal Apr 10, 2020 •

edited

johnmcollier Apr 10, 2020 •

edited

maysunfaisal commented Apr 10, 2020

johnmcollier commented Apr 15, 2020

maysunfaisal commented Apr 15, 2020

maysunfaisal commented Apr 15, 2020

kadel commented Apr 15, 2020

openshift-ci-robot commented Apr 15, 2020

openshift-ci-robot commented Apr 15, 2020

openshift-bot commented Apr 15, 2020

openshift-bot commented Apr 15, 2020

johnmcollier commented Apr 15, 2020

kadel commented Apr 21, 2020

Implement a basic, first pass odo push with Docker support #2806

Implement a basic, first pass odo push with Docker support #2806

Conversation

johnmcollier commented Apr 2, 2020 • edited

amitkrout commented Apr 5, 2020

johnmcollier commented Apr 6, 2020

codecov bot commented Apr 6, 2020 • edited

Codecov Report

maysunfaisal left a comment

Choose a reason for hiding this comment

maysunfaisal left a comment

Choose a reason for hiding this comment

maysunfaisal Apr 10, 2020

Choose a reason for hiding this comment

johnmcollier Apr 10, 2020

Choose a reason for hiding this comment

maysunfaisal Apr 10, 2020

Choose a reason for hiding this comment

johnmcollier Apr 10, 2020

Choose a reason for hiding this comment

maysunfaisal Apr 10, 2020 • edited

Choose a reason for hiding this comment

johnmcollier Apr 10, 2020 • edited

Choose a reason for hiding this comment

maysunfaisal commented Apr 10, 2020

johnmcollier commented Apr 15, 2020

maysunfaisal commented Apr 15, 2020

maysunfaisal commented Apr 15, 2020

kadel commented Apr 15, 2020

openshift-ci-robot commented Apr 15, 2020

openshift-ci-robot commented Apr 15, 2020

openshift-bot commented Apr 15, 2020

openshift-bot commented Apr 15, 2020

johnmcollier commented Apr 15, 2020

kadel commented Apr 21, 2020

Implement a basic, first pass `odo push` with Docker support #2806

Implement a basic, first pass `odo push` with Docker support #2806

johnmcollier commented Apr 2, 2020 •

edited

codecov bot commented Apr 6, 2020 •

edited

maysunfaisal Apr 10, 2020 •

edited

johnmcollier Apr 10, 2020 •

edited