-
Notifications
You must be signed in to change notification settings - Fork 151
feat(devfile): Generates devfile.yaml in addition to meta.yaml for che-editors and che-plugins #853
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #853 +/- ##
==========================================
+ Coverage 91.72% 92.41% +0.69%
==========================================
Files 38 40 +2
Lines 955 1042 +87
Branches 108 129 +21
==========================================
+ Hits 876 963 +87
Misses 79 79
Continue to review full report at Codecov.
|
sleshchenko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The only stuff that is missing: volumes components.
Comparing to devfile v1, volume mount is not enough, volume component should be declared explicitly, like for theia:
components:
- name: plugins
volume: {}
- name: theia-local
volume: {}
- name: remote-endpoint
volume:
ephemeral: trueThen plugins will not re-declare volume but just rely on the ones from Che Theia.
|
@sleshchenko yes thanks, Angel told me about volumes yesterday. Is that ephemeral is supported right now ? |
|
From Devfile API perspective - they are supported, from DevWorkspace Operator - they will be soon devfile/devworkspace-operator#281 |
|
@sleshchenko ok but DevWorkspace Operator won't reject the file if ephemeral is used ? |
I tried Angel's sample with ephemeral volume and no error is reported, workspace is started with PVC. |
3d997cd to
6399952
Compare
|
PR updated with volumes generation |
sleshchenko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
maybe it also makes sense to add devfile.yaml links into index.json
…e-editors and che-plugins Change-Id: Ibefbb93bc1eab00cdbfc1fe1e2ed2caac87166ce Signed-off-by: Florent Benoit <fbenoit@redhat.com>
|
devfile links added |
What does this PR do?
Write devfile.yaml in addition to meta.yaml for che-plugins and che-editors
Screenshot/screencast of this PR
What issues does this PR fix or reference?
Fixes eclipse-che/che#19060
How to test this PR?
Look at generated results in
latestfolder for example, it should include both meta.yaml and devfile.yamlPR Checklist
As the author of this Pull Request I made sure that:
What issues does this PR fix or referenceandHow to test this PRcompletedReviewers
Reviewers, please comment how you tested the PR when approving it.
Change-Id: Ibefbb93bc1eab00cdbfc1fe1e2ed2caac87166ce
Signed-off-by: Florent Benoit fbenoit@redhat.com