-
Notifications
You must be signed in to change notification settings - Fork 145
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
Specify che-editors.yaml in devfile v2 format #914
Conversation
Codecov Report
@@ Coverage Diff @@
## master #914 +/- ##
==========================================
+ Coverage 92.60% 93.00% +0.40%
==========================================
Files 40 42 +2
Lines 1068 1130 +62
Branches 136 148 +12
==========================================
+ Hits 989 1051 +62
Misses 79 79
Continue to review full report at Codecov.
|
} | ||
|
||
interface CheEditorProjectYaml { | ||
git?: CheEditorVCSYaml; |
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.
we don't need VCS
it's always git
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.
renamed to CheEditorGitYaml
starterProjects?: CheEditorProjectYaml[]; | ||
} | ||
|
||
export interface CheEditorYaml { |
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.
since last week, we now have proper types for devfile API
We should get rid of all *Yaml objects defining the Devfile API as now https://github.com/devfile/api is exposed at https://www.npmjs.com/package/@devfile/api
import { V1alpha2DevWorkspace } from '@devfile/api';
Idea of using the devfile definition to specify editors is to rely on existing @devfile/api
.d.ts struct and get new updates automatically
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.
This requires quite big refactoring, so I think we can do it in another iteration.
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.
👍 to doing it in another iteration
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.
I'm fine to make it as a new iteration
Signed-off-by: Igor Vinokur <ivinokur@redhat.com>
displayName: theia-ide | ||
description: Eclipse Theia, get the latest release each day. | ||
icon: https://raw.githubusercontent.com/theia-ide/theia/master/logo/theia-logo-no-text-black.svg?sanitize=true | ||
attributes: |
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.
based on devfile/api#352 (comment)
attributes in metadata are deprecated so probably need to move them to editors/attributes and using a freeForm format
as it's targeting plugin-registry only, maybe
schemaVersion: 2.1.0
attributes:
-pluginRegistry:
- publisher: eclipse
- version: next
- repository: https://github.com/eclipse-che/che-theia
or we remove it from the definition of the devfile
editors:
- firstPublicationDate:
publisher: eclipse
version: next
repository:
editor:
schemaVersion: 2.1.0
components:....
or we ask to not deprecate attributes in metadata (or we ask to have optional repository, firstPublicationDate, etc fields)
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.
According to devfile/api#352 (comment) top level attributes are going to be added at the same time when the metadata attributes are deprecated. So I propose to leave it as it is but when the devfile API is updated, move the metadata attributes to top level attributes
@benoitf any other concerns? |
@ericwill I was waiting for happy path being happy again (it's in progress) |
also, do you have tested generated devfile.yaml with devworkspace operator ? |
I relaunched happy path and it's working if would be a good exercice to test it on devWorkspace operator |
@benoitf I don't know how to use devWorkspace operator with Che. I've tested it by building an image from this PR and referring it in |
I suppose this one can wait to be merged at the beginning of next sprint? |
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.
we're at the beginning of the sprint, let's merge
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: benoitf, vinokurig The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
New changes are detected. LGTM label has been removed. |
Signed-off-by: Igor Vinokur ivinokur@redhat.com
What does this PR do?
Converts che-editors.yaml to use devfile v2 format.
Screenshot/screencast of this PR
What issues does this PR fix or reference?
fixes eclipse-che/che#19034
fixes #889
How to test this PR?
Che editors must work as before.
PR Checklist
As the author of this Pull Request I made sure that:
What issues does this PR fix or reference
andHow to test this PR
completedReviewers
Reviewers, please comment how you tested the PR when approving it.