If clonePath is set, sync to it #2747

johnmcollier · 2020-03-23T14:26:02Z

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

What type of PR is this?

Uncomment only one /kind line, and delete the rest.
For example, > /kind bug would simply become: /kind bug

/kind bug

What does does this PR do / why we need it:
This PR updates the sync code to sync to the devfile's clonePath folder if it's set. Certain devfiles, such as the official Go devfile, rely on this field being set properly (see https://github.com/eclipse/che-devfile-registry/blob/5a41b4b0c85893a6ee99059fcceab35b8a71780c/devfiles/go/devfile.yaml#L46 for example)

Which issue(s) this PR fixes:

Fixes #2741

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

johnmcollier · 2020-03-23T19:36:18Z

/retest

codecov · 2020-03-23T22:18:06Z

Codecov Report

Merging #2747 into master will decrease coverage by 0.32%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master    #2747      +/-   ##
==========================================
- Coverage   43.72%   43.39%   -0.33%     
==========================================
  Files          87       91       +4     
  Lines        8068     8233     +165     
==========================================
+ Hits         3528     3573      +45     
- Misses       4195     4309     +114     
- Partials      345      351       +6

Impacted Files	Coverage Δ
...g/devfile/adapters/kubernetes/component/adapter.go	`35.6% <80%> (+1.41%)`	⬆️
pkg/kclient/deployments.go	`34.66% <0%> (-54.99%)`	⬇️
pkg/component/watch.go	`67.33% <0%> (-6.67%)`	⬇️
pkg/application/application.go	`41.26% <0%> (-6.01%)`	⬇️
pkg/occlient/occlient.go	`52.69% <0%> (-0.1%)`	⬇️
pkg/lclient/images.go	`60% <0%> (ø)`
pkg/lclient/fakeclient.go	`42.62% <0%> (ø)`
pkg/lclient/client.go	`0% <0%> (ø)`
pkg/lclient/containers.go	`88.88% <0%> (ø)`
... and 1 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 fcd770f...e8bf177. Read the comment docs.

amitkrout · 2020-03-25T11:05:32Z

@johnmcollier Can you please add steps for how to validate this change with an example. Later we can think upon the possibility of integration test script.

johnmcollier · 2020-03-26T15:43:04Z

@amitkrout I've added instructions on how to validate.

How to validate:

Download devfile with a project that uses the clonePath field (try https://raw.githubusercontent.com/eclipse/che-devfile-registry/5a41b4b0c85893a6ee99059fcceab35b8a71780c/devfiles/go/devfile.yaml)
Pull down the project specified in the devfile and put in the same folder as the devfile (as Add ability to create application resource/template from stack #2582 hasn't been implemented)
Build my branch
Run odo push
Verify the project gets synced to /projects/<clonePath> in the remote container

rajivnathan

Change looks good to me, thanks! lgtm

kadel · 2020-03-26T17:17:30Z

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

+		// If the clonepath is set to a value, set it to be the sync folder
+		// As some devfiles rely on the code being synced to the folder in the clonepath
+		if project.ClonePath != nil {
+			return filepath.ToSlash(filepath.Join(kclient.OdoSourceVolumeMount, *project.ClonePath))


There should be a check to prevent escaping /projects or more precisely escaping from kclient.OdoSourceVolumeMount ( using ..
see: https://github.com/redhat-developer/devfile/blob/master/docs/devfile.md#clonepath

Hmm, good catch.

How should we handle that? Che throws an error if the clonePath starts with either / or .. (https://github.com/eclipse/che/blob/420d4ea34cf50e5835027016211f16b02eba8a54/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/devfile/convert/ProjectConverter.java#L109). We should probably do the same.

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

johnmcollier · 2020-03-26T18:13:55Z

@kadel Thanks for the review! I've updated the PR with changes to only allow relative clonePaths and to not allow .. in the clonePath.

kadel · 2020-03-26T18:15:49Z

pkg/devfile/adapters/kubernetes/component/adapter_test.go

 	projectNames := []string{"some-name", "another-name"}
 	projectRepos := []string{"https://github.com/some/repo.git", "https://github.com/another/repo.git"}
+	projectClonePath := "src/github.com/golang/example/"
+	invalidClonePaths := []string{"/var", "../var", "pkg/../../var"}


kadel · 2020-03-26T18:16:14Z

/approve

openshift-ci-robot · 2020-03-26T18:16:34Z

[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

rajivnathan

Adding lgtm label back since Tomas' comments have been addressed.

openshift-bot · 2020-03-26T20:28:34Z

/retest