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.

Sign up for GitHub

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

Jump to bottom

There is no Java code inside the IDE.JSP so there is no need to have all JSP stuff #6174

Merged
merged 1 commit into from
Sep 11, 2017
Merged

There is no Java code inside the IDE.JSP so there is no need to have all JSP stuff #6174

merged 1 commit into from
Sep 11, 2017

Conversation

benoitf
Copy link
Contributor

@benoitf benoitf commented Sep 7, 2017

What does this PR do?

Rename IDE.jsp to IDE.html as we only need to serve the html page.
so we can get rid of all the JSP libraries.

What issues does this PR fix or reference?

#6169

Changelog

Avoid to use JSP for IDE html page that has no java code.

Release Notes

N/A

Docs PR

N/A

Change-Id: I0c881fc37aedcc22f09fbbf16e49ce43f2dea334
Signed-off-by: Florent BENOIT fbenoit@redhat.com

@benoitf benoitf added kind/bug Outline of a bug - must adhere to the bug report template. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. team/osio labels Sep 7, 2017
@benoitf benoitf self-assigned this Sep 7, 2017
@benoitf benoitf requested a review from riuvshin as a code owner September 7, 2017 10:01
@benoitf benoitf mentioned this pull request Sep 7, 2017
Closed
4 tasks
@codenvy-ci
Copy link

Build # 3578 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/3578/ to view the results.

@benoitf
There is no Java code inside the IDE.JSP so there is no need to have …
c858b65 
…all JSP stuff.

We only need to serve the html page.

Change-Id: I0c881fc37aedcc22f09fbbf16e49ce43f2dea334
Signed-off-by: Florent BENOIT <fbenoit@redhat.com>
@codenvy-ci
Copy link

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/3591/

@skabashnyuk
Copy link
Contributor

Why do we need IDEController?

@benoitf
Copy link
Contributor Author

benoitf commented Sep 8, 2017

@skabashnyuk it's to fulfil this pattern https://github.com/eclipse/che/blob/c858b65b70ffa2686c5084cab8cf2ec25bd3ac08/assembly/assembly-ide-war/src/main/webapp/WEB-INF/web.xml#L63-L66

@skabashnyuk
Copy link
Contributor

What blocks us from remove servlet at all and use static html from war file?

@benoitf
Copy link
Contributor Author

benoitf commented Sep 8, 2017

@skabashnyuk I didn't wanted to be intrusive on this pull request. (I only replace JSP by a servlet)
Maybe we can remove the servlet and use a filter to do the redirect but I thought it was more a deep change on the logic.
But I agree that removing the servlet at all would be a nice improvement.

@benoitf benoitf added kind/enhancement A feature request - must adhere to the feature request template. and removed kind/bug Outline of a bug - must adhere to the bug report template. labels Sep 8, 2017
@benoitf
Copy link
Contributor Author

benoitf commented Sep 8, 2017

@vparfonov @riuvshin is it ok for you ? you want to wait like on monday ?

@gazarenkov
Copy link
Contributor

gazarenkov commented Sep 10, 2017 •
edited
Loading

@benoitf
since you edit web.xml
maybe you can also remove

<security-role>
       <description>the user role</description>
       <role-name>developer</role-name>
 </security-role>

pretty sure it is not used anymore no?

@benoitf
Copy link
Contributor Author

benoitf commented Sep 10, 2017

@gazarenkov yes when I've edited file I was thinking the same. I will issue another pull request as I don't like mixing stuff in PR

@benoitf benoitf merged commit bded571 into master Sep 11, 2017
@benoitf benoitf deleted the no-jsp branch September 11, 2017 08:53
@benoitf benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Sep 11, 2017
@benoitf benoitf added this to the 5.18.0 milestone Sep 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@riuvshin riuvshin riuvshin approved these changes

@vparfonov vparfonov vparfonov approved these changes

@skabashnyuk skabashnyuk skabashnyuk approved these changes

Assignees

@benoitf benoitf

Labels
kind/enhancement A feature request - must adhere to the feature request template.
Projects
None yet
Milestone
5.18.0
Development

Successfully merging this pull request may close these issues.
6 participants
@benoitf @codenvy-ci @skabashnyuk @gazarenkov @vparfonov @riuvshin