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.

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

Switch to use parser library #4188

Conversation

yangcao77
Copy link
Contributor

@yangcao77 yangcao77 commented Nov 3, 2020

What type of PR is this?

/area devfile
/kind code-refactoring

What does does this PR do / why we need it:
This PR removes the parser pkg from odo repo, and use parser from devfile/library instead
This PR also integrate odo with the devfile go structs defined in devfile/api

Which issue(s) this PR fixes:

Fixes #4117

PR acceptance criteria:

How to test changes / Special notes to the reviewer:
run integration tests and unit tests to make sure no regression has been introduced

@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. area/devfile-spec Issues or PRs related to the Devfile specification and how odo handles and interprets it. kind/code-refactoring labels Nov 3, 2020
@yangcao77 yangcao77 marked this pull request as draft November 3, 2020 19:18
@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 Nov 5, 2020
Signed-off-by: Stephanie <yangcao@redhat.com>
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Nov 9, 2020
@yangcao77 yangcao77 marked this pull request as ready for review November 9, 2020 16:38
Signed-off-by: Stephanie <yangcao@redhat.com>
@yangcao77 yangcao77 changed the title [WIP]Switch to use parser library Switch to use parser library Nov 9, 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 Nov 9, 2020
Signed-off-by: Stephanie <yangcao@redhat.com>
Signed-off-by: Stephanie <yangcao@redhat.com>
@yangcao77
Copy link
Contributor Author

This PR is currently blocked by: devfile/api#203
the yaml tags need to be added in structs for devfile writer to work

@kadel
Copy link
Member

kadel commented Nov 12, 2020

@yangcao77 It looks like there is something broken in headlight the paths (/ vs \)
Windows unit tests are failing on

--- FAIL: TestCopyDirWithFS (0.00s)
    --- FAIL: TestCopyDirWithFS/case_1:_folder_with_a_nested_file_and_folder (0.00s)
        util_test.go:2286: MoveDir() error = chmod C:\Users\travis\AppData\Local\Temp\destination/blah.js: file does not exist, wantErr false
        util_test.go:2340: some folder were not created
        util_test.go:2344: some files were not created
--- FAIL: TestGitSubDir (0.00s)
    --- FAIL: TestGitSubDir/case_1:_normal_sub_dir_exist (0.00s)
        util_test.go:2570: GitSubDir() error = chmod C:\Users\travis\AppData\Local\Temp\destination\java/main.java: file does not exist, wantErr false
        util_test.go:2611: all files were not copied

Notice the forward flash instead of back slash in the path.

@yangcao77
Copy link
Contributor Author

@kadel that is wired.. I did not modify those two functions / files. I will rebase with the latest master to see.

Signed-off-by: Stephanie <yangcao@redhat.com>
Signed-off-by: Stephanie <yangcao@redhat.com>
Signed-off-by: Stephanie <yangcao@redhat.com>
@codecov
Copy link

codecov bot commented Nov 16, 2020

Codecov Report

Merging #4188 (dbc7921) into master (0dea26a) will decrease coverage by 0.87%.
The diff coverage is 21.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4188      +/-   ##
==========================================
- Coverage   42.56%   41.69%   -0.88%     
==========================================
  Files         156      140      -16     
  Lines       13266    12783     -483     
==========================================
- Hits         5647     5330     -317     
+ Misses       7017     6887     -130     
+ Partials      602      566      -36     
Impacted Files Coverage Δ
pkg/component/component.go 22.44% <ø> (ø)
pkg/component/component_full_description.go 0.00% <0.00%> (ø)
pkg/component/devfile_repr.go 0.00% <ø> (ø)
pkg/config/env_var.go 47.82% <0.00%> (ø)
pkg/devfile/adapters/common/command_simple.go 81.39% <ø> (ø)
pkg/devfile/adapters/common/command_supervisor.go 0.00% <ø> (ø)
pkg/devfile/adapters/common/generic.go 16.25% <0.00%> (ø)
pkg/devfile/adapters/docker/component/adapter.go 56.98% <0.00%> (ø)
pkg/devfile/adapters/docker/utils/utils.go 85.13% <ø> (ø)
pkg/devfile/adapters/helper.go 8.00% <ø> (ø)
... and 27 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 2e8304e...ac74a29. Read the comment docs.

@kadel
Copy link
Member

kadel commented Nov 17, 2020

This PR is currently blocked by: devfile/api#203
the yaml tags need to be added in structs for devfile writer to work

@yangcao77 I see the tests are passing. Is this PR still blocked on devfile/api#203?

@yangcao77
Copy link
Contributor Author

@kadel It's unblocked now. people can start reviewing the PR.

@kadel
Copy link
Member

kadel commented Nov 18, 2020

/retest

pkg/config/config_test.go Outdated Show resolved Hide resolved
@kadel
Copy link
Member

kadel commented Nov 18, 2020

Good job @yangcao77
/approve

@openshift-ci-robot
Copy link
Collaborator

[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:

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 Nov 18, 2020
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.

CI is green
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Nov 20, 2020
@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-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Nov 20, 2020
@yangcao77
Copy link
Contributor Author

@mik-dass Can you help add back the lgtm label? the label was removed as I did a rebase

@mik-dass
Copy link
Contributor

@mik-dass Can you help add back the lgtm label? the label was removed as I did a rebase

Sure.
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Nov 21, 2020
@openshift-merge-robot openshift-merge-robot merged commit adaa1b4 into redhat-developer:master Nov 21, 2020
@rm3l rm3l added the area/refactoring Issues or PRs related to code refactoring label Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. area/devfile-spec Issues or PRs related to the Devfile specification and how odo handles and interprets it. area/refactoring Issues or PRs related to code refactoring lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch to use the parser from devfile/parser in odo repo
7 participants