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

fix: complete support of che-editor.yaml file #396

Merged
merged 1 commit into from
Nov 10, 2021
Merged

fix: complete support of che-editor.yaml file #396

merged 1 commit into from
Nov 10, 2021

Conversation

benoitf
Copy link
Contributor

@benoitf benoitf commented Nov 8, 2021

What does this PR do?

Handle id, reference, inline and override properties

What issues does this PR fix or reference?

Fixes eclipse-che/che#20715

Is it tested? How?

Tested with default samples and in addition:

Release Notes

Docs PR

Handle id, reference and inline and override properties
Fixes eclipse-che/che#20715
@github-actions
Copy link

github-actions bot commented Nov 8, 2021

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-396

@che-bot
Copy link
Contributor

che-bot commented Nov 8, 2021

✅ E2E dashboard tests succeed 🎉

See Details

Test product:

  • Use comment "[dashboard-ci-test]" to rerun the dashboard e2e tests

Eclipse Che QE channel: https://mattermost.eclipse.org/eclipse/channels/eclipse-che-qe

@benoitf

This comment has been minimized.

@codecov
Copy link

codecov bot commented Nov 8, 2021

Codecov Report

Merging #396 (0c295e8) into main (82e01c0) will decrease coverage by 0.18%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #396      +/-   ##
==========================================
- Coverage   48.81%   48.63%   -0.19%     
==========================================
  Files         207      207              
  Lines        7180     7207      +27     
  Branches     1203     1214      +11     
==========================================
  Hits         3505     3505              
- Misses       3301     3328      +27     
  Partials      374      374              
Impacted Files Coverage Δ
...orkspace-client/devworkspace/devWorkspaceClient.ts 41.95% <0.00%> (-4.38%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82e01c0...0c295e8. Read the comment docs.

@benoitf
Copy link
Contributor Author

benoitf commented Nov 8, 2021

it would be nice to have it included in the upcoming tag 7.39

repositoryEditorYamlUrl = cheEditorYaml.reference;
}
if (repositoryEditorYamlUrl) {
const response = await this.axios.get<string>(repositoryEditorYamlUrl, {
Copy link
Contributor

@olexii4 olexii4 Nov 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will cause 'has been blocked by CORS policy' error as we see with your example:

...
reference: https://eclipse-che.github.io/che-plugin-registry/main/v3/plugins/eclipse/che-theia/next/devfile.yaml
...

It should be done on the dashboard-backend

2021-11-09_13_35_43
.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@olexii4 it's strange, github.io is allowing CORS
requests are returning access-control-allow-origin: * and it was working on my side

Copy link
Contributor

@olexii4 olexii4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this proposal. I have tested it. It works except CORS error which happens in the case with external reference (e.q. https://eclipse-che.github.io/che-plugin-registry/main/v3/plugins/eclipse/che-theia/next/devfile.yaml).

I agree to merge it if we open an issue to add tests for this important functionality (because we already have one regression eclipse-che/che#20737 with P1 and we should prevent it in the future).

@benoitf
Copy link
Contributor Author

benoitf commented Nov 9, 2021

I agree to merge it if we open an issue to add tests for this important functionality (because we already have one regression eclipse-che/che#20737 with P1 and we should prevent it in the future).

@olexii4 it's not already handled by eclipse-che/che#20733 ?

@olexii4
Copy link
Contributor

olexii4 commented Nov 9, 2021

@olexii4 it's not already handled by eclipse-che/che#20733 ?

It is about IDE loading(IDE-loader) it is not cowering the Factory flow. But OK. I will add a comment to this issue.

@openshift-ci
Copy link

openshift-ci bot commented Nov 10, 2021

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: akurinnoy, benoitf, olexii4

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@benoitf benoitf merged commit e656113 into main Nov 10, 2021
@benoitf benoitf deleted the CHE-20715 branch November 10, 2021 07:31
@che-bot che-bot added this to the 7.39 milestone Nov 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

handle all fields of repository che-editor.yaml file
4 participants