Implement clonePath, update source code sync location#3907

Merged

openshift-merge-robot merged 5 commits intoredhat-developer:masterfrom

adisky:update-sync-location

Sep 15, 2020

Contributor

adisky commented Sep 8, 2020 •

edited

Loading

What type of PR is this?
/kind feature

What does does this PR do / why we need it:
If a single project is present in devfile:

if project.clonePath is present sync to $PROJECTS_ROOT/clonePath
if clonePath not set sync to $PROJECTS_ROOT/projectName

if no project or multiple project present in devfile

sync to $PROJECTS_ROOT

The value of $PROJECTS_ROOT is determined by sourceMapping field in devfile or PROJECTS_ROOT env variable. if both are unset default is /projects.

Which issue(s) this PR fixes:

Fixes #3729

PR acceptance criteria:

Unit test
Integration test
Documentation
I have read the test guidelines

How to test changes / Special notes to the reviewer:

create nodejs component from file https://github.com/openshift/odo/pull/3907/files#diff-233e1a3be07da905fb5e3d944f1e3398
verify the source code sync location with kubectl exec -it <pod-name> -- ls $PROJECTS_ROOT/<clonePathValue>

openshift-ci-robot added the kind/feature label

openshift-ci-robot requested review from GeekArthur and prietyc123

September 8, 2020 10:25

adisky force-pushed the update-sync-location branch from af3381e to 1c31cba Compare

September 8, 2020 10:34

adisky requested a review from johnmcollier

September 8, 2020 10:54

Member

johnmcollier commented Sep 8, 2020

@adisky Do we plan on supporting devfiles where multiple projects have clonePath set at some point? My concern is that we'll close out #3729 with this PR, but we still won't support that scenario.

codecov bot commented Sep 8, 2020 •

edited

Loading

Codecov Report

Merging #3907 into master will increase coverage by 0.03%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master    #3907      +/-   ##
==========================================
+ Coverage   44.82%   44.86%   +0.03%     
==========================================
  Files         143      143              
  Lines       13906    13919      +13     
==========================================
+ Hits         6234     6245      +11     
- Misses       7082     7083       +1     
- Partials      590      591       +1

Impacted Files	Coverage Δ
pkg/sync/adapter.go	`78.08% <85.71%> (+0.63%)`	⬆️

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 4e79790...d469775. Read the comment docs.

johnmcollier approved these changes

View reviewed changes

Member

johnmcollier left a comment

LGTM.

As discussed on slack, I'll add a note to #3798 about properly supporting multiple projects once this is merged.

openshift-ci-robot assigned johnmcollier

openshift-ci-robot added the lgtm label

Contributor

girishramnani commented Sep 9, 2020

/approve

Collaborator

openshift-ci-robot commented Sep 9, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: girishramnani

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [girishramnani]

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

openshift-ci-robot added the approved label

adisky force-pushed the update-sync-location branch from 1c31cba to 9fa5f3d Compare

September 9, 2020 10:49

openshift-ci-robot removed the lgtm label

johnmcollier approved these changes

View reviewed changes

Member

johnmcollier left a comment

LGTM

openshift-ci-robot added the lgtm label

openshift-bot commented Sep 9, 2020

/retest

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

12 similar comments

openshift-bot commented Sep 9, 2020

/retest

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

openshift-bot commented Sep 9, 2020

/retest

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

openshift-bot commented Sep 9, 2020

/retest

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

openshift-bot commented Sep 9, 2020

/retest

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

openshift-bot commented Sep 9, 2020

/retest

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

openshift-bot commented Sep 9, 2020

/retest

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

openshift-bot commented Sep 9, 2020

/retest

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

openshift-bot commented Sep 9, 2020

/retest

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

openshift-bot commented Sep 9, 2020

/retest

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

openshift-bot commented Sep 9, 2020

/retest

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

openshift-bot commented Sep 9, 2020

/retest

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

openshift-bot commented Sep 9, 2020

/retest

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

openshift-bot commented Sep 9, 2020

/retest

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

5 similar comments

openshift-bot commented Sep 9, 2020

/retest

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

openshift-bot commented Sep 9, 2020

/retest

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

openshift-bot commented Sep 9, 2020

/retest

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

openshift-bot commented Sep 9, 2020

/retest

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

openshift-bot commented Sep 9, 2020

/retest

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

Member

johnmcollier commented Sep 9, 2020

Okay, so this is my bad, I didn’t realize this PR also added a test devfile for the integration test.

@adisky can you update your PR to use the new devfile format in the integration test that you’re adding, since #3872 was merged? Looks like the tests will still fail until it’s updated.

/hold

openshift-ci-robot added the do-not-merge/hold label

adisky force-pushed the update-sync-location branch from 159276a to e807b1b Compare

September 10, 2020 07:29

openshift-ci-robot removed the lgtm label

Contributor Author

adisky commented Sep 10, 2020

/hold cancel

openshift-ci-robot removed the do-not-merge/hold label

mik-dass reviewed

View reviewed changes

tests/examples/source/devfiles/nodejs/devfile-with-multiple-projects.yaml Outdated Show resolved Hide resolved

adisky force-pushed the update-sync-location branch from e807b1b to ad6f5f5 Compare

September 10, 2020 10:35

mik-dass reviewed

View reviewed changes

tests/integration/devfile/cmd_devfile_push_test.go Outdated

+              			// devfile with clonePath set in project field
+              			helper.CopyExampleDevFile(filepath.Join("source", "devfiles", "nodejs", "devfile-with-projects.yaml"), filepath.Join(context, "devfile.yaml"))
+              			// we do not need to check source code location inside the container, as the expected workingDir
+              			// is already set in devfile.yaml, odo push would fail if it is not synced to correct location

Contributor

mik-dass Sep 10, 2020

I think odo push doesn't fail in case of nodejs components even if there is no package.json. I feel it's better to check for the presence of some files at the source location.

Contributor Author

adisky Sep 11, 2020

Done.

adisky force-pushed the update-sync-location branch from ad6f5f5 to 021a3b9 Compare

September 11, 2020 09:05

Contributor Author

adisky commented Sep 11, 2020

/retest


Container test exited with code 1, reason Error
---
[must-gather      ] OUT Get https://api.ci-op-wk349ril-2f74e.origin-ci-int-aws.dev.rhcloud.com:6443/apis/image.openshift.io/v1/namespaces/openshift/imagestreams/must-gather: dial tcp 34.232.159.141:6443: connect: connection refused
[must-gather      ] OUT 
[must-gather      ] OUT Using must-gather plugin-in image: quay.io/openshift/origin-must-gather:latest
The connection to the server api.ci-op-wk349ril-2f74e.origin-ci-int-aws.dev.rhcloud.com:6443 was refused - did you specify the right host or port?
error: failed to execute wrapped command: exit status 1

Member

kadel commented Sep 14, 2020

/retest

girishramnani added the required-for-v2 label

Aditi Sharma added 5 commits

September 14, 2020 17:43


          Update the source code sync location

ae3e7b6

if project.clonePath is present sync to $PROJECTS_ROOT/clonePath
if clonePath not set sync to $PROJECTS_ROOT/projectName
if no project present sync to $PROJECTS_ROOT


          Rebase and update unit test as per new changes

583d813


          Update the devfile as per new schema

0cad431


          check sync location from pod in integration tests

8f70b70


          Remove mistakenly added file

d469775

adisky force-pushed the update-sync-location branch from 914b329 to d469775 Compare

September 14, 2020 12:13

mik-dass approved these changes

View reviewed changes

Contributor

mik-dass left a comment

/lgtm

openshift-ci-robot assigned mik-dass

openshift-ci-robot added the lgtm label

openshift-merge-robot merged commit 6017b45 into redhat-developer:master

rm3l added the v2 label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

GeekArthur Awaiting requested review from GeekArthur

prietyc123 Awaiting requested review from prietyc123

2 more reviewers

johnmcollier johnmcollier approved these changes

mik-dass mik-dass approved these changes

Labels

approved kind/feature lgtm v2