Skip to content

Add support for SourceMapping field in devfiles #3295

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
Merged
merged 15 commits into from
Jul 3, 2020
Merged

Add support for SourceMapping field in devfiles #3295

merged 15 commits into from
Jul 3, 2020

Conversation

johnmcollier
Copy link
Member

@johnmcollier johnmcollier commented Jun 3, 2020 •
edited
Loading

What type of PR is this?
/kind feature
/area devfileV2

What does does this PR do / why we need it:
This PR adds support for the new SourceMapping field in devfile v2. When it is set, odo will mount the project source volume to that folder, rather than /projects or /projects/<projectName>.

Which issue(s) this PR fixes:

Fixes #2935

How to test changes / Special notes to the reviewer:
Use this sample V2 devfile and verify that it syncs to /test:

schemaVersion: 2.0.0
metadata:
  name: nodejs
  version: 1.0.0
projects:
  - name: nodejs-web-app
    git:
      location: "https://github.com/odo-devfiles/nodejs-ex.git"
components:
  - container:
      name: runtime
      image: quay.io/eclipse/che-nodejs10-ubi:nightly
      memoryLimit: 1024Mi
      mountSources: true
      sourceMapping: /test
      endpoints:
        - name: "nodejs"
          configuration:
            discoverable: false
            public: true
            protocol: tcp
            scheme: http
          targetPort: 3000
commands:
  - exec:
      id: devBuild
      component: runtime
      commandLine: npm install
      workingDir: /test/app
      group:
        kind: build
        isDefault: true
  - exec:
      id: devRun
      component: runtime
      commandLine: nodemon app.js
      workingDir: /test/app
      group:
        kind: run
        isDefault: true

I've also updated the odo push integration tests for Kube and Docker to test this scenario.

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation area/devfileV2 labels Jun 3, 2020
@johnmcollier johnmcollier reopened this Jun 3, 2020
@johnmcollier johnmcollier changed the title [WIP] Add support for SourceMapping field in devfiles Add support for SourceMapping field in devfiles Jun 5, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Jun 5, 2020
@johnmcollier
Copy link
Member Author

Removing WIP tag now that integration tests have been added

@johnmcollier
Fix unit test
f39fd4f 
Signed-off-by: John Collier <[email protected]>
@johnmcollier
Fix unit tests
807dea0 
Signed-off-by: John Collier <[email protected]>
prietyc123
prietyc123 reviewed Jun 8, 2020
View reviewed changes
tests/examples/source/devfilesV2/nodejs/devfileSourceMapping.yaml Outdated
@@ -0,0 +1,40 @@
schemaVersion: 2.0.0
metadata:
name: nodejs
Copy link
Contributor

Choose a reason for hiding this comment

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

I can see only devfileSourceMapping.yaml for nodejs component type. So i think this pr only handles for nodejs type and later other supported devfile component type will be added, right ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The sample devfile I used is nodejs, but the implementation of sourceMapping isn't specific to any devfile or runtime, so other devfiles don't need to be added, as it would be testing the same functionality.

prietyc123
prietyc123 suggested changes Jun 8, 2020
View reviewed changes
tests/integration/devfile/cmd_devfile_push_test.go
@@ -83,6 +83,31 @@ var _ = Describe("odo devfile push command tests", func() {
helper.CmdShouldPass("odo", "push", "--devfile", "devfile.yaml", "--project", namespace)
})

It("checks that odo push works with a devfile with sourcemapping set", func() {
helper.CmdShouldPass("odo", "create", "nodejs", "--project", namespace, cmpName)
Copy link
Contributor

@prietyc123 prietyc123 Jun 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I am not confused why we are only looking for nodejs? Why not springboot ?

Copy link
Member Author

@johnmcollier johnmcollier Jun 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is required since --devfile isn't supported in odo create (see #3118), so this is a necessary step for all tests for devfiles currently (see the remainder of the tests in that file). So we have to run odo create first, then copy over the devfile we need to test.

metacosm
metacosm approved these changes Jun 9, 2020
View reviewed changes
Copy link
Contributor

@metacosm metacosm 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 Jun 9, 2020
@metacosm metacosm mentioned this pull request Jun 9, 2020
Closed
@johnmcollier
Copy link
Member Author

/retest

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Jun 14, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jun 22, 2020
@openshift-bot
Copy link

/retest

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

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

@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
Fix tests
60bfd6e 
Signed-off-by: John Collier <[email protected]>
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jul 2, 2020
@johnmcollier
Fix docker test
ba3e108 
Signed-off-by: John Collier <[email protected]>
@girishramnani
Copy link
Contributor

/lgtm
As the code is fine and tests got resolved too

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

/retest

@openshift-bot
Copy link

/retest

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

2 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-merge-robot openshift-merge-robot merged commit 20f84de into redhat-developer:master Jul 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prietyc123 prietyc123 prietyc123 requested changes

@metacosm metacosm metacosm approved these changes

@GeekArthur GeekArthur Awaiting requested review from GeekArthur

@girishramnani girishramnani Awaiting requested review from girishramnani

Assignees

@metacosm metacosm

@dharmit dharmit

@girishramnani girishramnani

@adisky adisky

@maysunfaisal maysunfaisal

@prietyc123 prietyc123

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Specify the source mapping path for component containers