Make the parser able to parse any file (including files with .yml extension)

[rm3l]

What does this PR do?:

This makes sure the Devfile parser does not make any assumptions on the path it is provided. It should now be able to parse any file path it is provided by clients, including files with a .yml extension.

After digging, I found no special place in the code where the library is discovering a Devfile.

Which issue(s) this PR fixes:

Fixes devfile/api#1265

PR acceptance criteria:

Testing and documentation do not need to be complete in order for this PR to be approved. We just need to ensure tracking issues are opened and linked to this PR, if they are not in the PR scope due to various constraints.

• Unit/Functional tests
• QE Integration test
• Documentation (READMEs, Product Docs, Blogs, Education Modules, etc.)
• Client Impact

I checked the following clients:

• odo: there is an ongoing PR (Recognize devfile.yml and .devfile.yml as Devfiles redhat-developer/odo#7130). odo has a specified list of acceptable filenames for a Devfile, and this list has been extended to include devfile.yml and .devfile.yml

• ODC: they call the parser using the Data field, which should not be affected by the changes here (which handles parsing from a specified devfile path): https://github.com/openshift/console/blob/master/pkg/devfile/handler.go#L55. I tested based on the instructions here.

• Che: they have their own logic for parsing: devfile/devworkspace-operator@main/pkg/library
  So they should not be impacted by the changes here. Furthermore, they currently look by default for a devfile with the file name .devfile.yaml or devfile.yaml. For other cases, they allow users to specify a custom path, which also covers {,.}devfile.yml or any other files. I tested on Dev Sandbox using a devfile.yml file and it works without any problem.
  See eclipse.dev/che/docs/stable/end-user-guide/url-parameter-for-the-devfile-file-name and che-incubator/che-code@main/code/extensions/che-remote/src/extension.ts#L100-L112

• HAS: [DEVHAS-493]support devfile.yml redhat-appstudio/application-service#400 made it possible to allow devfile.yml and .devfile.yml as potential input devfiles. But parsing of those files currently fails. I checked with the changes here and parsing works as expected.

• Gosec scans

How to test changes / Special notes to the reviewer:

• Parsing a context dir containing multiple devfiles. It should be able to detect and pick a devfile based on the following priority order: devfile.yaml > .devfile.yaml > devfile.yml > .devfile.yml

$ mkdir -p /tmp/test-parser
$ wget https://registry.devfile.io/devfiles/java-maven/1.3.0 -O /tmp/test-parser/devfile.yml

# Try to parse it. It should pass now.
# Previously, it would fail with the following message:
# failed to populateAndParseDevfile: 
#   failed to read devfile from path '/tmp/test-parser/devfile.yaml': 
#   open /tmp/test-parser/devfile.yaml: not a directory
$ go run . /tmp/test-parser

parsing devfile from /tmp/test-parser
schema version: 2.1.0
All component: 
tools
m2
All Exec commands: 
command mvn-package is with kind: build
workingDir is: ${PROJECT_SOURCE}
command run is with kind: run
workingDir is: ${PROJECT_SOURCE}
command debug is with kind: debug
workingDir is: ${PROJECT_SOURCE}
=========================================================
Container components applied filter: 
Exec commands applied filter: 
err: Attribute with key "alpha.build-dockerfile" does not exist
dockerfilePath: 

• Parsing any file, regardless of file name or extension

# Create devfile file on the local filesystem
# and name it however you want (e.g., devfile.yml)
$ wget https://registry.devfile.io/devfiles/go/1.0.2 -O /tmp/my-devfile.yml

# Try to parse it. It should pass now.
# Previously, it would fail with the following message:
# failed to populateAndParseDevfile: 
#   failed to read devfile from path '/tmp/devfile.yml/devfile.yaml': 
#   open /tmp/devfile.yml/devfile.yaml: not a directory
$ go run . /tmp/devfile.yml

parsing devfile from /tmp/devfile.yml
schema version: 2.1.0
All component: 
runtime
All Exec commands: 
command build is with kind: build
workingDir is: ${PROJECT_SOURCE}
command run is with kind: run
workingDir is: ${PROJECT_SOURCE}
=========================================================
Container components applied filter: 
Exec commands applied filter: 
err: Attribute with key "alpha.build-dockerfile" does not exist
dockerfilePath: 

[codecov]

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (393d29e) 60.84% compared to head (728befe) 61.05%.
Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #186      +/-   ##
==========================================
+ Coverage   60.84%   61.05%   +0.20%     
==========================================
  Files          41       42       +1     
  Lines        5006     5017      +11     
==========================================
+ Hits         3046     3063      +17     
+ Misses       1767     1761       -6     
  Partials      193      193              

Files	Coverage Δ
pkg/devfile/parser/context/location.go	100.00% <100.00%> (ø)
pkg/devfile/parser/parse.go	64.71% <ø> (ø)
pkg/devfile/parser/writer.go	54.28% <100.00%> (ø)
pkg/devfile/parser/context/context.go	35.21% <0.00%> (+2.74%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[thepetk]

I think the idea behind this solution makes sense.

One question I have is what about Che and HAS? Have we checked how they are first filtering the files passed to ParseDevfileAndValidate?

[rm3l]

Whoops, I just noticed this comment..

I think the idea behind this solution makes sense.

One question I have is what about Che and HAS? Have we checked how they are first filtering the files passed to ParseDevfileAndValidate?

I checked Che, and in my understanding, Che (the DevWorkspace Operator) has its own library for parsing Devfiles: https://github.com/devfile/devworkspace-operator/tree/main/pkg/library
So they should not be impacted by the changes here. Furthermore, they currently look by default for a devfile with the file name .devfile.yaml or devfile.yaml. For other cases, they allow users to specify a custom path, which also covers {,.}devfile.yml or any other files. I tested on Dev Sandbox using a devfile.yml file and it works without any problem.
See https://eclipse.dev/che/docs/stable/end-user-guide/url-parameter-for-the-devfile-file-name/ and https://github.com/che-incubator/che-code/blob/main/code/extensions/che-remote/src/extension.ts#L100-L112

This PR is more about making sure the parser is able to parse filesystem paths other than devfile.yaml and checking whether there are places where the library discovers devfiles, which does not seem to be the case.
Seeing that all tools seem to have their own logic for detecting default devfile file names (other than devfile.yaml) and that .yml is not supported by all tools, I was thinking of mentioning this point in the documentation (along with the recommended priority order if multiple variants of devfiles are present). I plan to do that in a subsequent PR.

As for HAS, I saw this has been covered too in DEVHAS-493 (redhat-appstudio/application-service#400), where support for detecting {,.}devfile.yml has been added.
@yangcao77 Could you please confirm that on HAS?

[rm3l]

I've gone ahead and tried to check the changes here on HAS.
I thought recognizing [.]devfile.yml was already implemented on HAS (via redhat-appstudio/application-service#400), but when trying to run the unit tests under cdq-analysis (TestScanRepo) against a multi-component repo with devfile.yml files (https://github.com/rm3l/rh-app-svc-devfile-test-multi-components-deep), I ran into the same error as the one we are trying to address in this PR.

=== RUN   TestScanRepo
--- FAIL: TestScanRepo (1.21s)
=== RUN   TestScanRepo/Should_return_2_devfile_contexts,_and_2_devfileURLs_as_this_is_a_multi_comp_devfile
    devfile_test.go:297: 
got unexpected error invalid devfile due to err: 
failed to populateAndParseDevfile: 
failed to read devfile from path '/tmp/testclone/devfile-sample-java-springboot-basic/.devfile/devfile.yml/devfile.yaml': 
open /tmp/testclone/devfile-sample-java-springboot-basic/.devfile/devfile.yml/devfile.yaml: 
not a directory, 
failed to parse the devfile content from /tmp/testclone/devfile-sample-java-springboot-basic/.devfile/devfile.yml
    --- FAIL: TestScanRepo/Should_return_2_devfile_contexts,_and_2_devfileURLs_as_this_is_a_multi_comp_devfile (1.21s)
FAIL

(As I expected, the test passes with the changes in this PR)

@yangcao77 Am I missing anything about DEVHAS-493 and redhat-appstudio/application-service#400?

I guess HAS will need to update its version of the Devfile library once the changes here are merged..

[yangcao77]

the failure you saw is due to this:

library/pkg/devfile/parser/context/context.go

Lines 99 to 101 in 36607c5

	if !strings.HasSuffix(d.relPath, ".yaml") {
	if _, err := os.Stat(filepath.Join(d.relPath, "devfile.yaml")); os.IsNotExist(err) {
	if _, err := os.Stat(filepath.Join(d.relPath, ".devfile.yaml")); os.IsNotExist(err) {

in library, if the extension is not .yaml , it will append devfile.yaml at the end of the path. so HAS passed /tmp/testclone/devfile-sample-java-springboot-basic/.devfile/devfile.yml to library and try to parse the content. then library thinks devfile.yml is not supported devfile extention, and append devfile.yaml .
so you see the error message:

failed to read devfile from path '/tmp/testclone/devfile-sample-java-springboot-basic/.devfile/devfile.yml/devfile.yaml': open /tmp/testclone/devfile-sample-java-springboot-basic/.devfile/devfile.yml/devfile.yaml: not a directory, failed to parse the devfile content from /tmp/testclone/devfile-sample-java-springboot-basic/.devfile/devfile.yml

I saw in your PR, you removed the block which appends devfile.yaml , it will fix the current issue, but I’m not suggesting to do it. it will cause regression to existing users who only pass the context path to library

and also to test with your changes in library with HAS and ODC(please do), need to replace the library version in go.mod

replace github.com/devfile/library/v2 => https://github.com/rm3l/devfile-library <the commit go version>

[yangcao77]

I guess HAS will need to update its version of the Devfile library once the changes here are merged..

Yes, HAS need the change in library to be able to parse the content.

[rm3l]

the failure you saw is due to this:

library/pkg/devfile/parser/context/context.go

Lines 99 to 101 in 36607c5

	if !strings.HasSuffix(d.relPath, ".yaml") {
	if _, err := os.Stat(filepath.Join(d.relPath, "devfile.yaml")); os.IsNotExist(err) {
	if _, err := os.Stat(filepath.Join(d.relPath, ".devfile.yaml")); os.IsNotExist(err) {

in library, if the extension is not .yaml , it will append devfile.yaml at the end of the path. so HAS passed /tmp/testclone/devfile-sample-java-springboot-basic/.devfile/devfile.yml to library and try to parse the content. then library thinks devfile.yml is not supported devfile extention, and append devfile.yaml . so you see the error message:

failed to read devfile from path '/tmp/testclone/devfile-sample-java-springboot-basic/.devfile/devfile.yml/devfile.yaml': open /tmp/testclone/devfile-sample-java-springboot-basic/.devfile/devfile.yml/devfile.yaml: not a directory, failed to parse the devfile content from /tmp/testclone/devfile-sample-java-springboot-basic/.devfile/devfile.yml

I saw in your PR, you removed the block which appends devfile.yaml , it will fix the current issue, but I’m not suggesting to do it. it will cause regression to existing users who only pass the context path to library

#186 (comment)

and also to test with your changes in library with HAS and ODC(please do), need to replace the library version in go.mod

replace github.com/devfile/library/v2 => https://github.com/rm3l/devfile-library <the commit go version>

Yes, that's how I am testing this with odo for example (with go mod edit -replace ...).

[yangcao77]

change looks good to me. assumes verified it works with consumers

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rm3l, 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:

• OWNERS [rm3l]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[rm3l]

change looks good to me. assumes verified it works with consumers

Thanks for the review!
Yes, I manually checked that this works with consumers and updated the issue description accordingly for reference. For ODC specifically, I manually tested the changes here based on the instructions here.