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

allow .devfile.yaml #14548

Closed
maxandersen opened this issue Sep 13, 2019 · 22 comments · Fixed by #17537
Closed

allow .devfile.yaml #14548

maxandersen opened this issue Sep 13, 2019 · 22 comments · Fixed by #17537
Assignees
Labels
area/che-server good first issue Community, this issue looks easy to start with for a new contributor. Just take it. We'll help you! kind/enhancement A feature request - must adhere to the feature request template. new&noteworthy For new and/or noteworthy issues that deserve a blog post, new docs, or emphasis in release notes severity/P1 Has a major impact to usage or development of the system. status/open-for-dev An issue has had its specification reviewed and confirmed. Waiting for an engineer to take it.
Milestone

Comments

@maxandersen
Copy link

devfile.yaml is not really something you want to have listed in your project explorer or listed in directory constantly.

Would be great if che would look for .devfile.yaml if devfile.yaml is not found.

@maxandersen maxandersen added the kind/enhancement A feature request - must adhere to the feature request template. label Sep 13, 2019
@che-bot che-bot added the status/need-triage An issue that needs to be prioritized by the curator responsible for the triage. See https://github. label Sep 13, 2019
@ibuziuk ibuziuk added severity/P2 Has a minor but important impact to the usage or development of the system. area/devfile and removed status/need-triage An issue that needs to be prioritized by the curator responsible for the triage. See https://github. labels Sep 14, 2019
@skabashnyuk skabashnyuk added this to the 7.4.0 milestone Sep 19, 2019
@skabashnyuk skabashnyuk modified the milestones: 7.4.0, Backlog - Platform Oct 10, 2019
@ibuziuk ibuziuk added good first issue Community, this issue looks easy to start with for a new contributor. Just take it. We'll help you! status/open-for-dev An issue has had its specification reviewed and confirmed. Waiting for an engineer to take it. labels Mar 2, 2020
@maxandersen
Copy link
Author

@ibuziuk i tried have a quick look and I can't find where che searches for devfile...can you give a pointer ?

@sunix sunix added severity/P1 Has a major impact to usage or development of the system. and removed severity/P2 Has a minor but important impact to the usage or development of the system. labels May 5, 2020
@maxandersen
Copy link
Author

does not look like .withDevfileFilename() allows setting multiple possible locations.

But if we add a fallback at https://github.com/eclipse/che/blob/8c2be9a8f4aa16377d430e141bd30ca62aad4ede/wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/urlfactory/URLFactoryBuilder.java#L117
which looks for same path but with a "." at the front it could work from any URL import.

@skabashnyuk
Copy link
Contributor

if @l0rd don't mind I think we can take this task to the next sprint. We need to agree on the order of the search. Is this correct order correct, any addition?

  • devfile.yaml
  • .devfile.yaml

@l0rd
Copy link
Contributor

l0rd commented May 6, 2020

Yes let's take this for next sprint, I think we owe it to @maxandersen

@gorkem
Copy link
Contributor

gorkem commented May 6, 2020

Let this dot be known as the maxandersen dot from this day forth. :)

@kadel
Copy link

kadel commented May 11, 2020

What if both .devfile.yaml and devfile.yaml is present?
Based on the discussion it looks like that .devfile.yaml will get silently ignored and devfile.yaml will be used. It might not be a common situation but it could happen by mistake and then it will get quite confusing for users.

I don't think that it is a bad thing to have a devfile file always visible.

@sunix
Copy link
Contributor

sunix commented May 11, 2020

@kadel at least we give the choice to the project team which files it wish to work with .devfile.yaml or devfile.yaml.
If that situation is happening, i guess someone in the team would fix it eventually... it is not a big deal in my opinion.

@kadel
Copy link

kadel commented May 11, 2020

How about at least showing a warning when there are both devfile.yaml and .devfile.yaml?

@maxandersen Why it is a problem when devfile.yaml shows up in the file explorer?

@JPinkney
Copy link
Contributor

Isn't this something you could technically just use a setting for?

"files.exclude": {
       "**/devfile.yaml": true
}

and the devfile won't appear in the project explorer

@sunix
Copy link
Contributor

sunix commented May 11, 2020

some projects maybe opened by different IDEs ... and I guess we don't want to pollute a project view with all the files for Jenkins, Che, VSCode, travis, .github, etc .... what ever the IDE you are using, it is a user preference to hide these files or not

@kadel
Copy link

kadel commented May 14, 2020

What about Windows? Are the dotfiles hidden there also by default?

@maxandersen
Copy link
Author

@maxandersen Why it is a problem when devfile.yaml shows up in the file explorer?

every time I generated project templates over the last 15 years there are an uproar when these files are included by default. They only are accepted if they are universally used across the development team - i.e. Dockerfile are a recent addition which been accepted as okey - but devfile will at least initially be used by a mere fraction of users of our standard projects thus adding it front and center will make people complain.

I'll keep it simple - if you want quarkus and other middleware runtimes to include support for devfile for che and odo by default it needs to be possible to hide it by default.

@maxandersen
Copy link
Author

"files.exclude": {
"**/devfile.yaml": true
}

that means we or the user need to include such config out of the box of every IDE (adding even more files) and it will still be visible by default by default in a shell.

@maxandersen
Copy link
Author

What about Windows? Are the dotfiles hidden there also by default?

yes they are. you don't see .git, .gitignore, .dockerignore, .vscode, .intellij, .eclipse neither.

@kadel
Copy link

kadel commented Jun 8, 2020

@kadel at least we give the choice to the project team which files it wish to work with .devfile.yaml or devfile.yaml.

If we allow this then tools need to error out when both .devfile.yaml and devfile.yaml are present! Otherwise, this will lead to a lot of confusion and problems.

problem scenario

  • I have a project that uses .devfile.yaml
  • Someone doesn't notice that there already is .devfile.yaml and adds new devfile.yaml
  • Now there are two devfiles each might be different.
  • If tooling just priorities devfile.yaml over .devfile.yaml the users that are used to editing .devfile.yaml will get confused about why their changes are not being reflected or vice versa.
  • Tooling needs to error out when both styles of devfiles are used.

@sparkoo
Copy link
Member

sparkoo commented Aug 6, 2020

Current PR uses list of devfile names that is sorted by priority, so if multiple devfiles are found, we take the one nearest to the top.

I've just blocked the PR, because based on discussions here, redhat-developer/odo#3126 and devfile/api#61, we need to reconsider that.
cc: @skabashnyuk @mshaposhnik @gorkem @kadel @l0rd @maxandersen

@gorkem
Copy link
Contributor

gorkem commented Aug 6, 2020

@sparkoo I do not think we should error. So if we discover both devfile.yaml and .devfile.yaml we use devfile.yaml but show a warning that simply states Both devfile.yaml and .devfile.yaml detected, using devfile.yaml.

@mshaposhnik
Copy link
Contributor

mshaposhnik commented Aug 6, 2020

We did that list of names configurable. So Che admin can add any amount of names he wants. But yes, in that case we need to check them all for presence. That's sounds logical, but may consume some workspace startup time. At the same time, dashboard indicates which file it picks for creating factory, so an attentive user should not fall into the trap.

@gorkem
Copy link
Contributor

gorkem commented Aug 6, 2020

Good point with the dashboard and the startup time. I think we can start with this on Che. I am not even sure if we even need the configuration.

@mshaposhnik
Copy link
Contributor

After some investigation, i found that we cannot show warning on factory accepting stage (at least without serious factory and dashb refactoring) :(
So i have created another one for dashb to make source filename more visible for the user: #17606
I think it should be enough for the time being.

Che Platform team sprint #188 automation moved this from In progress to Done Aug 11, 2020
@gorkem gorkem added the new&noteworthy For new and/or noteworthy issues that deserve a blog post, new docs, or emphasis in release notes label Aug 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/che-server good first issue Community, this issue looks easy to start with for a new contributor. Just take it. We'll help you! kind/enhancement A feature request - must adhere to the feature request template. new&noteworthy For new and/or noteworthy issues that deserve a blog post, new docs, or emphasis in release notes severity/P1 Has a major impact to usage or development of the system. status/open-for-dev An issue has had its specification reviewed and confirmed. Waiting for an engineer to take it.
Projects
No open projects
Development

Successfully merging a pull request may close this issue.