Add devfile parser #2500
Add devfile parser #2500
Conversation
openshift-ci-robot
added
do-not-merge/work-in-progress
pkg/devfile/parser/context.go
Outdated
| func (d *DevfileCtx) Validate() error { | ||
|
|
||
| // Validate devfile | ||
| if err := d.ValidateDevfileSchema(); err != nil { |
There was a problem hiding this comment.
can't we just return d.ValidateDevfileSchema()?
There was a problem hiding this comment.
Initially, I was expecting more validations as a part of Validate() func, but currently there is only one, so this can be done.
| RegistryUrl *string `yaml:"registryUrl,omitempty" json:"registryUrl,omitempty"` | ||
| } | ||
|
|
||
| type DevfileComponentOpenshift struct { |
There was a problem hiding this comment.
Can we reuse all these strings from 1.0.0, maybe even extract them into a common package? and at places where the 1.0.1 structs have more parameters, then can we embed the 1.0.0 struct in the respective 1.0.1 struct and add the new parameter in the 1.0.1 struct?
There was a problem hiding this comment.
This approach will help use use a common interface and not have branches in the layers of code which uses devfiles
There was a problem hiding this comment.
This might be actually a good idea. Not sure about a common package, but reusing structs from the previous version if they are not changes make actually sense. It will also nicely highlight the changes between versions.
There was a problem hiding this comment.
I have refactored the code to have common structs and common functions between different versions of devfile.
As per our discussion with the devfile team, since the 1.0.1-beta version has been merged into 1.0.0 version, I have removed the 1.0.1-beta files from the code as well.
kanchwala-yusuf
force-pushed
the
devfile-parser
branch
from
ecbd606
to
2abb937
Compare
| } | ||
|
|
||
| if !isDockerImageComponentPresent { | ||
| errMsg := fmt.Sprintf("odo requires atleast one component of type '%s' in devfile", DevfileComponentsTypeDockerimage) |
There was a problem hiding this comment.
can we inline this error message?
There was a problem hiding this comment.
Taken a different approach for this.
|
|
||
| // isProjectTypeSupported checks if the given project type is supported in ODO | ||
| func isProjectTypeSupported(typ DevfileProjectsType) bool { | ||
| for _, supportedType := range SupportedDevfileProjectTypes { |
There was a problem hiding this comment.
can we use the In function here?
https://github.com/openshift/odo/blob/adf3006360abc5cb707d9a9f442669989272f4e5/pkg/util/util.go#L80
There was a problem hiding this comment.
Not really, to use In() we may need to convert/typecast every element of SupportedDevfileProjectTypes of custom type DevfileProjectType to string.
kanchwala-yusuf
force-pushed
the
devfile-parser
branch
7 times, most recently
from
fa1255a
to
4c3fc64
Compare
|
/retest |
|
/restest |
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
/retest |
|
/test v4.1-integration-e2e-benchmark |
kanchwala-yusuf
force-pushed
the
devfile-parser
branch
from
4c3fc64
to
2b684b9
Compare
|
@kanchwala-yusuf Please don't amend old commits. It makes it really hard to review changes :-( Just push new one, the PRs get automatically squashed before it is merged to master, so number of commits in PR doesn't matter. |
|
[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 |
openshift-ci-robot
added
the
approved
| t.Helper() | ||
|
|
||
| // Create tempfile | ||
| f, err := ioutil.TempFile(os.TempDir(), TempJsonDevfilePrefix) |
There was a problem hiding this comment.
let's use the fake package here and write the stuff in memory instead of the disk. That will speed up the testing.
| func (d *DevfileCtx) SetDevfileContent() error { | ||
|
|
||
| // Read devfile | ||
| data, err := ioutil.ReadFile(d.absPath) |
There was a problem hiding this comment.
why not use the Fs Filesystem which is part of the DevfileCtx? What will help us with unit tests using the fake system.
|
|
||
| var ( | ||
| tempDevfile = createTempDevfile(t, validJsonRawContent100()) | ||
| d = DevfileCtx{absPath: tempDevfile.Name()} |
There was a problem hiding this comment.
We can pass the fake filesystem here in DevfileCtx
| t.Run("run command not present", func(t *testing.T) { | ||
|
|
||
| commands := []DevfileCommand{ | ||
| {Name: "run app"}, |
There was a problem hiding this comment.
since the description says run command not present, should this be build app?
| emptyApiVersionJson = `{"apiVersion":}` | ||
| ) | ||
|
|
||
| t.Run("valid apiVersion", func(t *testing.T) { |
There was a problem hiding this comment.
can we convert these tests to the tabular format we currently use in odo?
|
@mik-dass Good points on testing. Are you ok if @kanchwala-yusuf addresses this in followup PR? |
There was a problem hiding this comment.
@kanchwala-yusuf As this s a feature request, can you please define the acceptance criteria. it would be helpful for us to find out missing integration tests if any.
@kadel @kanchwala-yusuf Sure. Since #2553 is created to track the issue, I don't mind. |
@amitkrout , I am not sure if we have a user story for this. User stories started from this sprint. This issue was opened a couple of sprints back. |
|
/reopen |
|
@kanchwala-yusuf: Reopened this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Closed this by mistake, hence reopend it. |
|
/lgtm |
openshift-ci-robot
added
the
lgtm
cdrage
added
the
kind/feature
rm3l
added
the
estimated-size/XXL (60 - ??)
What type of PR is this?
What does does this PR do / why we need it:
This PR implements the basic components devfile parser.
Which issue(s) this PR fixes**:
Fixes #2477