use api override and merging utils to get flatterned devfile #48
Conversation
Signed-off-by: Maysun J Faisal <[email protected]>
Signed-off-by: Maysun J Faisal <[email protected]>
Signed-off-by: Stephanie <[email protected]>
Signed-off-by: Stephanie <[email protected]>
Signed-off-by: Stephanie <[email protected]>
Signed-off-by: Stephanie <[email protected]>
Signed-off-by: Stephanie <[email protected]>
|
blocked by: devfile/api#276 |
Signed-off-by: Stephanie <[email protected]>
Signed-off-by: Stephanie <[email protected]>
Signed-off-by: Stephanie <[email protected]>
pkg/devfile/parser/parse.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| parentData, err := ParseFromURL(parent.Uri) |
There was a problem hiding this comment.
One thing to think about is, you are calling func ParseFromURL with both parent and plugin. We need to avoid infinite loops if the parent or plugin references back to the main devfile. Do we allow such situations acc to the schema?
because it is parseDevfile(d, true) inside func ParseFromURL
There was a problem hiding this comment.
it would error out since reference back would result in duplication in flattened devfile?
Signed-off-by: Stephanie <[email protected]>
yangcao77
force-pushed
the
testOverrideFn-1
branch
from
47a1e44 to
70e85e6
Compare
There was a problem hiding this comment.
can we add a couple more test cases?
try a test case where there are
- multiple events of diff type in main, parent and plugin devfile - preStart1 preStart2 in main, preStart1, postStart1 in parent, postStart1, postSart2 in plugin, etc. and see if it holds up nicely. You can probably update the currrent test for events
- different type of overrides - for example, if the plugin devfile has a container component, what happens if the plugin overrides of the same name is of type volume component
- can we also add a test case for this use api override and merging utils to get flatterned devfile #48 (comment)
| Id: "devrun", | ||
| CommandUnion: v1.CommandUnion{ | ||
| Exec: &v1.ExecCommand{ | ||
| WorkingDir: "/projects/nodejs-starter", |
There was a problem hiding this comment.
this is a nit, but can we just randomize the commands for plugin and parent, so that other ppl looking at this dont get confused with how overriding works. They might think it should be the exact same. Could you pls do this for projects and component tests as well as other tests?
pkg/devfile/parser/parse_test.go
Outdated
| wantErr: true, | ||
| }, | ||
| { | ||
| name: "case 14: error out if the same command is defined in both plugin devfile and parent devfile", |
There was a problem hiding this comment.
case 14 number is repeated twice even tho its a diff test
pkg/devfile/parser/parse_test.go
Outdated
| wantErr: true, | ||
| }, | ||
| { | ||
| name: "case 13: error out if the same project is defined again in the local devfile", |
There was a problem hiding this comment.
nit: can we say name: "case 13: error out if the plugin project is defined again in the local devfile",
There was a problem hiding this comment.
Also this is dup with case 7 tho case 7 is with parent, same for case 4
Signed-off-by: Stephanie <[email protected]>
Signed-off-by: Stephanie <[email protected]>
Signed-off-by: Stephanie <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: maysunfaisal, yangcao77 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 |
What does this PR do?
use api override and merging utils to get flatterned devfile
this pr also adds plugin components parsing
What issues does this PR fix or reference?
devfile/api#198
devfile/api#240
Is your PR tested? Consider putting some instruction how to test your changes
unit test created