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.

Sign up for GitHub

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

Jump to bottom

Implement clonePath, update source code sync location #3907

Merged
merged 5 commits into from
Sep 15, 2020
Merged

Implement clonePath, update source code sync location #3907

merged 5 commits into from
Sep 15, 2020

Conversation

adisky
Copy link
Contributor

@adisky 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:

How to test changes / Special notes to the reviewer:

@openshift-ci-robot openshift-ci-robot added the kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation label Sep 8, 2020
@johnmcollier
Copy link
Member

@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
Copy link

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%.

Impacted file tree graph

@@            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
johnmcollier approved these changes Sep 8, 2020
View reviewed changes
Copy link
Member

@johnmcollier johnmcollier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

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

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Sep 8, 2020
@girishramnani
Copy link
Contributor

/approve

@openshift-ci-robot
Copy link
Collaborator

[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

Needs approval from an approver in each of these files:

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 openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Sep 9, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Sep 9, 2020
johnmcollier
johnmcollier approved these changes Sep 9, 2020
View reviewed changes
Copy link
Member

@johnmcollier johnmcollier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Sep 9, 2020
@openshift-bot
Copy link

/retest

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

12 similar comments
@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

5 similar comments
@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@johnmcollier
Copy link
Member

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 openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Sep 9, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Sep 10, 2020
@adisky
Copy link
Contributor Author

adisky commented Sep 10, 2020

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Sep 10, 2020
mik-dass
mik-dass reviewed Sep 10, 2020
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@adisky
Copy link
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

@kadel
Copy link
Member

kadel commented Sep 14, 2020

/retest

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
mik-dass
mik-dass approved these changes Sep 15, 2020
View reviewed changes
Copy link
Contributor

@mik-dass mik-dass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Sep 15, 2020
@openshift-merge-robot openshift-merge-robot merged commit 6017b45 into redhat-developer:master Sep 15, 2020
@rm3l rm3l added the v2 Issue or PR that applies to the v2 of odo label Jun 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mik-dass mik-dass mik-dass approved these changes

@johnmcollier johnmcollier johnmcollier approved these changes

@GeekArthur GeekArthur Awaiting requested review from GeekArthur

@prietyc123 prietyc123 Awaiting requested review from prietyc123

Assignees

@johnmcollier johnmcollier

@mik-dass mik-dass

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation lgtm Indicates that a PR is ready to be merged. Required by Prow. v2 Issue or PR that applies to the v2 of odo
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

clonePath should be supported in odo
9 participants
@adisky @johnmcollier @girishramnani @openshift-ci-robot @openshift-bot @mik-dass @kadel @rm3l @openshift-merge-robot