[WIP] Made several changes in Devfile format #2
Conversation
|
I don't have permissions to request a review in this repository. |
There was a problem hiding this comment.
Thank you very much @sleshchenko for this PR. This really helps.
petclinic.yaml
Outdated
| - name: spring-boot | ||
| port: 8080 | ||
| mountProjectsSources: true |
There was a problem hiding this comment.
I understand that you remove that because you have moved it to the volumes section. But the problem with this approach is that the user need to know Che internals (i.e. that Che does the cloning in /projects/) and ideally I would like to avoid that.
So while I am perfectly fine if we don't implement mountProjectSources in Phase 1 I would want us to implement it in the future.
There was a problem hiding this comment.
(i.e. that Che does the cloning in /projects/)
It doesn't clone projects to /projects/ but it clones projects to volume with name projects. So, a user is able to specify any mountPath he wishes.
I removed mountProjectSources to have the same format as che-plugin uses. We can consider adding mountProjectSources to both of Devfile and che-plugin. Or you think that it is more critical for Devfile but it is not an issue for che-plugin.yaml?
There was a problem hiding this comment.
it clones projects to volume with name projects. So, a user is able to specify any mountPath he wishes.
I am probably a n00b Che user but I wasn't aware of this convention. And we should make things easier to understand for n00bs like me.
I agree with you that che_plugin.yaml and devfile should share the same spec so +1 to update/review both models. I think this example has helped us to identify some problems with che_plugin.yaml and that's an opportunity to improve it. I have created eclipse-che/che#12249 to track that.
petclinic.yaml
Outdated
| - name: spring-boot | ||
| port: 8080 | ||
| mountProjectsSources: true | ||
| public: true |
There was a problem hiding this comment.
We should be careful adding too many fields here. The main goal is to make it simple to write a Devfile. For instance fields public and attributes should be optional.
There was a problem hiding this comment.
question, why public is not an attribute (for example secure is an attribute) ?
There was a problem hiding this comment.
We should be careful adding too many fields here. The main goal is to make it simple to write a Devfile. For instance fields public and attributes should be optional.
Actually, public and attributes are optional fields. true is a default value for public field.
question, why public is not an attribute (for example secure is an attribute) ?
The same answer as for the next question #2 (comment).
So, let's discuss whether che-plugin.yaml and Devfile should use the same terms and structure or not. If yes, could we update che-plugin.yaml model with your proposals?
petclinic.yaml
Outdated
| port: 8080 | ||
| mountProjectsSources: true | ||
| public: true | ||
| targetPort: 8080 |
There was a problem hiding this comment.
why targetPort instead of port ? (I mean, if I have to write the file I prefer to use short names)
There was a problem hiding this comment.
Agree with you. Good question. But probably this question is more to @garagatyi @skabashnyuk @l0rd since I just copied che-plugin.yaml format.
We could have different formats for Devfile and che-plugin.yaml but I prefer to use the same terms not to confuse users each time when it switch between writing workspace configuration and plugin configuration.
So, we can consider updating the model of che-plugin.yaml before Che 7 Beta release.
petclinic.yaml
Outdated
| - name: maven-repo | ||
| containerPath: /root/.m2 | ||
| services: | ||
| mountPath: /root/.m2 |
There was a problem hiding this comment.
hello, what is the meaning of mountPath ?
For a volume it's always something mounted no ?
How is handled subpath BTW ?
There was a problem hiding this comment.
it is path where volume folder will be mounted inside of container
There was a problem hiding this comment.
in k8s I have the ability to specify subpath:
volumeMounts:
- mountPath: /var/lib/mysql
name: site-data
subPath: mysql
There was a problem hiding this comment.
@benoitf Sorry, I missed your third question.
It's right. subPath feature was not implemented. If it is important on this stage we can register an issue for add subPath feature to Che Volumes, now it has only name(key of map where it is stored) and mountPath (which is called path there) https://github.com/eclipse/che/blob/07263f1e30089689d71b057f747a44a29283e3c4/core/che-core-api-model/src/main/java/org/eclipse/che/api/core/model/workspace/config/Volume.java#L19
Does
it is path where volume folder will be mounted inside of container
answer your questions
what is the meaning of mountPath ?
For a volume it's always something mounted no ?
?
There was a problem hiding this comment.
thx @sleshchenko
maybe we could agree on the model first and then see which optional field would be or not implemented ?
There was a problem hiding this comment.
maybe we could agree on the model first and then see which optional field would be or not implemented ?
Sure. If we are talking about model I think it makes sense to consider adding subpath to CheVolume, che-plugin.yaml and Devfile.
There was a problem hiding this comment.
If we have subpath how we would implement that ono OSIO when subpaths already used for separating workspaces, volumes of workspaces. Would we also use the third level of nesting?
petclinic.yaml
Outdated
| containerPath: /root/.m2 | ||
| services: | ||
| mountPath: /root/.m2 | ||
| - name: projects |
There was a problem hiding this comment.
Agree with @l0rd it would be easier to not to write /projects and let that to the implementation to decide on which path it's coming)
There was a problem hiding this comment.
well, a user is able to specify any path he needs for projects folder to be mounted inside of his container.
There was a problem hiding this comment.
I'm not sure we're speaking of the same thing. User can specify all the mount volumes he might want. But if I mount /projects I expect that all ppl mounting projects source-tree will have the same data across the workspace and not get their own volume as well.
Also if plug-ins or IDE are using /projects folder but you mount /foobar then it will not behave as you are expecting as well.
There was a problem hiding this comment.
User can specify all the mount volumes he might want. But if I mount /projects I expect that all ppl mounting projects source-tree will have the same data across the workspace and not get their own volume as well.
Che Volumes should mount the same data if volumes name is the same.
And also the user is able to mount volume where he needs it to be mount inside of a container.
Example:
- name: mvn-stack
type: dockerImage
image: maven:3.5.4-jdk-8
entryPoint: sleep infinity
volumes:
- name: maven-repo
mountPath: /root/.m2
- name: gradle-stack
type: dockerImage
image: gradle:3.5.4-jdk-8
entryPoint: sleep infinity
volumes:
- name: maven-repo
mountPath: /home/user/.m2mvn-stack and gradle-stack tools should have the same data but mount to different paths.
It is how I understand the format described by @l0rd. Maybe it is not related to Che Volumes (what would be confusing) at all.
sleshchenko
force-pushed
the
devfileUpdate
branch
from
86096c8
to
ae0ae55
Compare
|
Part of changes are merged in #3 |
This PR contains the following changes that are in separate commits:
If reviewers are OK with proposed changes I will update plantuml diagrams and remove WIP status.