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

Add ASP.NET devfile #32

Merged
merged 4 commits into from
Sep 19, 2019
Merged

Add ASP.NET devfile #32

merged 4 commits into from
Sep 19, 2019

Conversation

tolusha
Copy link
Contributor

@tolusha tolusha commented Jul 12, 2019

Signed-off-by: Anatoliy Bazko abazko@redhat.com

What does this PR do?

Adds stack for developing ASP.NET Core web application
https://github.com/gothinkster/aspnetcore-realworld-example-app

What issues does this PR fix or reference?

eclipse-che/che#13529

Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
@tolusha tolusha self-assigned this Jul 12, 2019
@l0rd
Copy link
Contributor

l0rd commented Jul 12, 2019

cc @ibuziuk @amisevsk a new devfile to handle...

@sparkoo
Copy link
Member

sparkoo commented Jul 12, 2019

I'm getting lot of errors, no code navigation... I'm using che-theia:next

asp_err

@tolusha
Copy link
Contributor Author

tolusha commented Jul 12, 2019

@sparkoo @svor
Firstly try to install all dependencies.
So, follow the provided commands:

  1. Install Cake
  2. Build the project

@sparkoo
Copy link
Member

sparkoo commented Jul 12, 2019

trying that, but editor is still unhappy :(

@svor
Copy link
Contributor

svor commented Jul 12, 2019

@sparkoo It can be because of eclipse-che/che#13739
try to refresh the browser tab, after that omnisharp ls should pick up this project.

@skabashnyuk
Copy link
Contributor

404 is that expected?
Знімок екрана  о 13 54 05

@svor
Copy link
Contributor

svor commented Jul 12, 2019

@skabashnyuk preview URL should contain /swagger at the end.
@tolusha need to find a way how to configure previewUrl in devfile. You can see how it was implemented in the stack: https://github.com/eclipse/che/blob/2b0149a01c67738faa590214057b60214454c854/ide/che-core-ide-templates/src/main/resources/samples.json#L1094

@sparkoo
Copy link
Member

sparkoo commented Jul 15, 2019

devfile and it's tasks works for me.
One issue with URL as @skabashnyuk mentioned.
Second issue with editor as @svor mentioned the issue eclipse-che/che#13739. With this bug, user experience is terrible. The editor basically can't find any dependency and red-underline every reference, even to core .net api. After refresh, it loads properly and works. IMHO we can't have this in base samples. These must work out of the box, without any hurdles.

@tolusha
Copy link
Contributor Author

tolusha commented Jul 15, 2019

@skabashnyuk
It is expected because nothing is configured at the root path. As @svor mentioned we need to open a swagger page (see the screenshot below). But I don't see the way how to configure the previewUrl for commands [1]. Any ideas?

[1] https://redhat-developer.github.io/devfile/

@sparkoo
User is supposed to read the readme.md file before working on the project. The swagger URL is mentioned there. As a workaround it is possible to move the project to the che-samples and update the project by configuring some service at the root path and adding a link to the swagger page.

Screenshot from 2019-07-16 02-18-11

@sparkoo
Copy link
Member

sparkoo commented Jul 16, 2019

@tolusha how swagger helps with the issue?

@tolusha
Copy link
Contributor Author

tolusha commented Jul 16, 2019

@sparkoo
I meant there is no issue with 404. It is expected, because no services are configured on the root path.

@sparkoo
Copy link
Member

sparkoo commented Jul 16, 2019

@tolusha ah, ok. I thought you were speaking about second issue with plugin. Make sense now :)
I guess we don't want to fork the project and I don't know how to set something like previewUrl either. I guess this sample isn't the only one with this issue. We should probably go through all of them and check whether we're satisfied with user experience we're delivering.

@tolusha
Copy link
Contributor Author

tolusha commented Jul 19, 2019

@sparkoo @l0rd
So, what is our plan toward this PR? postponed?

@l0rd
Copy link
Contributor

l0rd commented Jul 19, 2019

For the previewUrl problem I guess we do not have other option than forking the real-world example as a che-sample and make the root URL redirect to the application URL.

I don't know if there are other problems.

@sparkoo
Copy link
Member

sparkoo commented Jul 19, 2019

@l0rd see my comment #32 (comment) There is issue with Omnisharp plugin eclipse-che/che#13739

@l0rd
Copy link
Contributor

l0rd commented Jul 19, 2019

Ok but eclipse-che/che#13739 is not related to this Devfile. That's about the support of any .NET project and is similar to eclipse-che/che#13427 (that's about Java support). It can be related to the size of the project (how long it takes to clone it): if the cloning takes tool long a refresh will be needed.

So @sparkoo you are suggesting not to include this sample until eclipse-che/che#13427 is fixed right? I think it makes sense. Maybe in the meantime we can find out a smaller sample that would work.

@sparkoo
Copy link
Member

sparkoo commented Jul 19, 2019

@l0rd yes, that's what I'm suggesting. Also C#.NET sample is affected by that issue, and I don't think we can go much simpler than that https://github.com/che-samples/dotnet-web-simple

@l0rd
Copy link
Contributor

l0rd commented Jul 19, 2019

@sparkoo ok I agree with you

@amisevsk
Copy link
Contributor

A number of existing devfiles also have the issue of the preview URL not opening to a path handled by the server (e.g. nodejs-mongo's preview url just gives {"errors":{"message":"Not Found","error":{}}}, PHP+MySQL serves a directory structure instead of the app).

Is validity of endpoints/preview URLs something I should add to our checklist for testing devfiles?

@sparkoo
Copy link
Member

sparkoo commented Jul 19, 2019

@amisevsk there is such a checklist? I've created an issue to retest and unify all devfiles once all are merged (eclipse-che/che#13863), are you going to do that?

@amisevsk
Copy link
Contributor

@sparkoo I did a relatively quick run through in testing #38, available here, and @ScrewTSW is doing similar for che.openshift.io: https://github.com/redhat-developer/che-functional-tests/issues/543#issuecomment-512795837

I personally have been glossing over preview URL / endpoint issues thus far, though. There are a number of potential improvements (e.g. some PHP devfiles do not expose the right port, the java stack exposes 8080 but runs java-console-simple, etc). My general bar has thus far been "I was able to make this work".

@sparkoo
Copy link
Member

sparkoo commented Jul 19, 2019

@amisevsk ok, I didn't know about this effort. My opinion is, that these are our samples and they must work perfectly. URL that is loaded after project start is public facing and very visible. It is highly probably that user will hit it, so I would consider it as a blocker issue.
Imagine you're trying Che for the first time, trying sample with your language of choice and you hit the issue. What is the chance you'll try your project and consider Che as your main IDE? When even samples from authors does not work 100% ...
That's only my opinion, though. It might not be that important.

@l0rd
Copy link
Contributor

l0rd commented Jul 22, 2019

@sparkoo perfectly agree with your idea. To address this now we have only one approach: modify/fork the sample to make them work with the current endpoint URL (with no subpath). I have created eclipse-che/che#13945 to support that better in the next sprints.

@amisevsk yes you should track the problems with the URL as well. And if we could automate the generation of the report and add it as a PR check it would be awesome 🙃

@ibuziuk
Copy link
Member

ibuziuk commented Jul 31, 2019

@l0rd since there are problems with URL / subpath could we avoid merging this PR for GA and postpone it till 7.1.0 ?

@l0rd
Copy link
Contributor

l0rd commented Aug 2, 2019

@ibuziuk yes that makes sense. But let's merge this if we are able to fix the URL problems in the samples before the GA.

Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
@tolusha
Copy link
Contributor Author

tolusha commented Aug 5, 2019

Image for dev container is updated.
Redirection to a swagger page is added. [1]

[1] https://github.com/che-samples/aspnetcore-realworld-example-app/blob/master/src/Conduit/Startup.cs#L146-L151

@tolusha
Copy link
Contributor Author

tolusha commented Aug 5, 2019

@ibuziuk
Is it possible to merge now?

@svor
Copy link
Contributor

svor commented Aug 5, 2019

Redirection to /swagger resources works fine on my side.

@ibuziuk
Copy link
Member

ibuziuk commented Aug 6, 2019

@tolusha I would rather postpone it to 7.1.0 similar to C / C++ one, wdyt ?

@tolusha
Copy link
Contributor Author

tolusha commented Aug 6, 2019

@ibuziuk
Ok it makes sense.

@tolusha tolusha changed the title Add ASP.NET devfile [DON'T MERGE 7.1.0 TARGET] Add ASP.NET devfile Aug 6, 2019
@tolusha tolusha changed the title [DON'T MERGE 7.1.0 TARGET] Add ASP.NET devfile Add ASP.NET devfile Sep 19, 2019
Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
@tolusha tolusha merged commit 80586f3 into master Sep 19, 2019
@tolusha tolusha deleted the ab/asp.net branch September 19, 2019 13:18
monaka added a commit that referenced this pull request Apr 29, 2020
Ohrimenko1988 added a commit that referenced this pull request Jun 5, 2020
Signed-off-by: Ihor Okhrimenko <iokhrime@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants