allow `.devfile.yaml` by anandrkskd · Pull Request #5138 · redhat-developer/odo

anandrkskd · 2021-10-12T11:34:56Z

Signed-off-by: anandrkskd anandrkskd@gmail.com

What type of PR is this?
/kind feature

What does this PR do / why we need it:

This PR adds a feature with which we can use .devfile.yaml without adding --devfile flag.
With this PR in, odo will prefer devfile.yaml and if not present will use .devfile.yaml if .devfile.yaml exists.

Which issue(s) this PR fixes:

Fixes #3126

PR acceptance criteria:

Unit test
Integration test
Documentation
I have read the test guidelines

How to test changes / Special notes to the reviewer:
Simplest way to check if .devfile.yaml is used without using --devfile flag

git clone https://github.com/openshift/nodejs-ex && cd nodejs-ex

odo create nodejs 

mv devfile.yaml .devfile.yaml

odo push

netlify · 2021-10-12T11:35:35Z

✔️ Deploy Preview for odo-docusaurus-preview ready!

🔨 Explore the source changes: 801451f

🔍 Inspect the deploy log: https://app.netlify.com/sites/odo-docusaurus-preview/deploys/617637b00131ac00073d0f62

😎 Browse the preview: https://deploy-preview-5138--odo-docusaurus-preview.netlify.app

anandrkskd · 2021-10-13T08:01:17Z

@dharmit, @feloy can you give early PR review?
The feature is working, all left for this PR is adding test and cleaning up the code.

feloy · 2021-10-13T11:38:26Z

@anandrkskd , I can see this error:

odo create nodejs
mv devfile.yaml .devfile.yaml
odo describe
 ✗  open /[...]/devfile.yaml: no such file or directory

In this case, it should be able to find the .devfile.yaml

valaparthvi · 2021-10-14T07:55:36Z

+	for _, devFile := range possibleDevfileNames {
+		if util.CheckPathExists(filepath.Join(contexDir, devFile)) {
+			return devFile
+		}
+	}


What happens if both the possible devfiles are present? Shouldn't we raise an error in that case? Which devfile gets precedence in case both are present?
And, would it help to sort the possibleDevfileNames before iterating over it? Can we ensure that every iteration on possibleDevfileNames follows the same order?

We discussed this point with Anand. He first wanted to raise an error if both files were present, I campaigned for not doing this, so we can have a more generic function, trying to find the file in a >2 number of different names (imagine we want to also check for the yml extension...). But the discussion is open...

An array is ordered and will always be iterated in the same order. The maps are not.

Can we set a preference key for the user to determine their default devfile? I remember Gerald asked(on the channel) if he could use devfile-v2.yaml as his default devfile without using --devfile flag.

Can we set a preference key for the user to determine their default devfile

We can do that, but I think it's a different feature than what this PR adds to odo.
I think we can have a different issue for adding that feature to odo.

+1, it should be a separate issue which requires its own discussion.

Can we set a preference key for the user to determine their default devfile? I remember Gerald asked(on the channel) if he could use devfile-v2.yaml as his default devfile without using --devfile flag.

I don't think that uses should be allowed to change default devfile. If they want to use something else then devfile.yaml or .devfile.yaml they can do and should use --devfile flag. Gerald's use case was specific and the only reason he needed this was to work around the fact that odo supports v2 but che doesn't

Based on the discussion above, I am okay with not raising an error, but I would prefer that we log the devfile that we are going to use.

valaparthvi · 2021-10-18T09:50:30Z

@anandrkskd Can you resolve merge conflicts here?

feloy · 2021-10-20T06:40:41Z

I have updated the tests to rename the devfile.yaml to .devfile.yaml just after odo create to check that all tests are still passing:

feloy#8

We probably don't want to keep these tests as is, as using .devfile.yaml is a minor behaviour and we don't want to miss errors when devfile is not renamed, and it would cost too much time to run all tests with both names.

/approve

@valaparthvi I let you lgtm when it is ok for you

openshift-ci · 2021-10-20T06:41:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feloy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [feloy]

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

feloy · 2021-10-20T14:36:01Z

@anandrkskd It would be nice to have a more complete test with a .devfile.yaml file, covering the most important odo commands

Signed-off-by: anandrkskd <anandrkskd@gmail.com>

sonarqubecloud · 2021-10-25T04:52:02Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
3.9% Duplication

anandrkskd · 2021-10-25T04:54:45Z

It would be nice to have a more complete test with a .devfile.yaml file, covering the most important odo commands

@feloy do you mean testcases covering service/link scenarios using .devfile.yaml ?

dharmit · 2021-10-25T06:37:43Z

/lgtm

@anandrkskd can you open a new issue for the tests as we discussed?

openshift-bot · 2021-10-25T06:44:37Z

/retest-required