Skip to content

Conversation

@vitaliy-guliy
Copy link
Contributor

What does this PR do?

Adds a sample with JetBrains IDEA editor

What issues does this PR fix or reference?

Is it tested? How?

PR Checklist

  • E2E tests pass (when PR is ready, comment /test v8-devworkspace-operator-e2e, v8-che-happy-path to trigger)
    • v8-devworkspace-operator-e2e: DevWorkspace e2e test
    • v8-che-happy-path: Happy path for verification integration with Che

@openshift-ci
Copy link

openshift-ci bot commented Feb 1, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link
Collaborator

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

Tested sample in an OpenShift 4.12 cluster, and it works as expected (with a few caveats):

  • Setting .spec.routingClass: che (with Eclipse Che installed in the cluster) results in the workspace working as expected
  • Using the basic routing class (default) results in the workspace failing to start. This is due to #1031 and not a problem with this PR; using the correct URL allows accessing the editor as expected.

In my testing, the experience of actually using the editor is fairly slow, with typing into a file taking multiple seconds at times, though I think this is an issue with the editor/setup and not the devfile itself.

@codecov
Copy link

codecov bot commented Feb 2, 2023

Codecov Report

Base: 50.17% // Head: 50.25% // Increases project coverage by +0.08% 🎉

Coverage data is based on head (45e5e0b) compared to base (27b6c98).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1030      +/-   ##
==========================================
+ Coverage   50.17%   50.25%   +0.08%     
==========================================
  Files          70       70              
  Lines        6021     6031      +10     
==========================================
+ Hits         3021     3031      +10     
+ Misses       2770     2769       -1     
- Partials      230      231       +1     
Impacted Files Coverage Δ
controllers/workspace/devworkspace_controller.go 62.16% <0.00%> (+0.57%) ⬆️
...r/devworkspacerouting/solvers/resolve_endpoints.go 33.33% <0.00%> (+5.91%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@vitaliy-guliy
Copy link
Contributor Author

I'm not sure whether it make any sense to have a sample command here as the IDEA knows nothing about devfile-based commands.

@vitaliy-guliy vitaliy-guliy marked this pull request as ready for review February 2, 2023 12:13
@openshift-ci openshift-ci bot requested a review from amisevsk February 2, 2023 12:14
@amisevsk
Copy link
Collaborator

amisevsk commented Feb 2, 2023

I'm not sure whether it make any sense to have a sample command here as the IDEA knows nothing about devfile-based commands.

That's a good point -- IDEA would need some sort of plugin or similar to understand devfile commands. They should likely be dropped for now.

Copy link
Collaborator

@AObuchow AObuchow left a comment

Choose a reason for hiding this comment

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

I've been trying to test this on OpenShift 4.12, but unfortunately the workspace hasn't been able to start up before the timeout yet after 3 tries.

Asides from minor comment, the actual DevWorkspace YAML looks good to me.

I recommend squashing all the commits for this PR into one once it is ready for a final review/merge.

@AObuchow
Copy link
Collaborator

AObuchow commented Feb 2, 2023

I've been trying to test this on OpenShift 4.12, but unfortunately the workspace hasn't been able to start up before the timeout yet after 3 tries.

Just realized I forgot to try this PR with #1032 applied 😅 will re-test

Copy link
Collaborator

@AObuchow AObuchow left a comment

Choose a reason for hiding this comment

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

Was able to get the sample working after applying #1032

Similar to @amisevsk the editor was kinda slow at first, but after a bit things sped up (still somewhat slow, but usable). I also installed the python editor and got the script to run.

Copy link
Collaborator

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

LGTM apart from the project name

@openshift-ci
Copy link

openshift-ci bot commented Feb 2, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk, AObuchow, vitaliy-guliy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@amisevsk
Copy link
Collaborator

amisevsk commented Feb 2, 2023

Before we can merge, commits should be squashed as appropriate and all need a signed-off-by footer.

@openshift-ci openshift-ci bot removed the lgtm label Feb 3, 2023
@openshift-ci
Copy link

openshift-ci bot commented Feb 3, 2023

New changes are detected. LGTM label has been removed.

Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>
@amisevsk amisevsk merged commit 48693aa into devfile:main Feb 3, 2023
@vitaliy-guliy vitaliy-guliy deleted the idea-editor-sample branch February 6, 2023 10:14
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.

4 participants