Skip to content
This repository has been archived by the owner on Jun 2, 2023. It is now read-only.

Addspringbootwebsocket #54

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

peiyang2024
Copy link

@peiyang2024 peiyang2024 commented Jun 21, 2016

Add spring websocket technology to Liberty Starter


This change is Reviewable

@katheris
Copy link
Contributor

Reviewed 1 of 27 files at r1.
Review status: 1 of 27 files reviewed at latest revision, 1 unresolved discussion.


README.md, line 37 [r1] (raw file):

Inside the application project there is a application.springboot.websocket package containing 4 classes:
* Application</code>: The entry point for the SpringBoot application.

This is inconsistent with the other spring microservices that put the Application class in application.spring and then the tech specific ones in the application.spring.tech package.


Comments from Reviewable

@katheris
Copy link
Contributor

Reviewed 8 of 27 files at r1.
Review status: 9 of 27 files reviewed at latest revision, 2 unresolved discussions.


starter-microservice-springboot-websocket/repository/0.0.1/provided-pom.xml, line 44 [r1] (raw file):

            <version>1.0.10</version>
        </dependency>
    </dependencies>

Are we missing websocket dependencies here?


Comments from Reviewable

@katheris
Copy link
Contributor

Reviewed 2 of 27 files at r1.
Review status: 11 of 27 files reviewed at latest revision, 3 unresolved discussions.


starter-microservice-springboot-websocket/repository/0.0.1/compile-pom.xml, line 39 [r1] (raw file):

        <dependency>
           <groupId>org.springframework.boot</groupId>
           <artifactId>spring-boot-starter-thymeleaf</artifactId>

What is this dependency for?


Comments from Reviewable

@katheris
Copy link
Contributor

Reviewed 2 of 27 files at r1.
Review status: 13 of 27 files reviewed at latest revision, 4 unresolved discussions.


starter-microservice-springboot-websocket/src/main/java/com/ibm/liberty/starter/service/springboot/websocket/api/v1/ProviderEndpoint.java, line 40 [r1] (raw file):

public class ProviderEndpoint {

    private static final String DEPENDENCY_URL = "http://localhost:9082/springboot/artifacts/net/wasdev/wlp/starters/springboot/websocket";

This string is not being used and should be removed.


Comments from Reviewable

@katheris
Copy link
Contributor

Reviewed 3 of 27 files at r1.
Review status: 16 of 27 files reviewed at latest revision, 5 unresolved discussions.


starter-microservice-springboot-websocket/src/main/webapp/sample/myProject-application/src/main/java/application/springboot/Application.java, line 26 [r1] (raw file):

@SpringBootApplication

public class Application extends SpringBootServletInitializer  {

The location of this file is incorrectly referenced in the readme, the readme should be updated to use the correct package.


Comments from Reviewable

@katheris
Copy link
Contributor

Reviewed 4 of 27 files at r1.
Review status: 20 of 27 files reviewed at latest revision, 5 unresolved discussions.


Comments from Reviewable

@katheris
Copy link
Contributor

Reviewed 1 of 27 files at r1.
Review status: 21 of 27 files reviewed at latest revision, 6 unresolved discussions.


starter-microservice-springboot-websocket/src/test/java/com/ibm/liberty/starter/service/springboot/websocket/api/v1/it/TestApplication.java, line 78 [r1] (raw file):

      assertNotNull("No response from API for sample", sample);
      assertNotNull("Expected locations", sample.getLocations());
      assertEquals("No files were expected for sample", 7, sample.getLocations().length);

The string in the assert should say "7 files were expected for sample".


Comments from Reviewable

@katheris
Copy link
Contributor

Reviewed 2 of 27 files at r1.
Review status: 23 of 27 files reviewed at latest revision, 7 unresolved discussions.


starter-microservice-springboot-websocket/src/main/webapp/WEB-INF/classes/description.html, line 22 [r1] (raw file):

  <p>Inside the application project there is a application.springboot.websocket package containing four classes:</p>
  <ul>
      <li><code>Application</code>: The entry point for the SpringBoot application</li>

This file lives in the application.springboot package. The description should be updated to reflect this.


Comments from Reviewable

@katheris
Copy link
Contributor

Review status: 23 of 27 files reviewed at latest revision, 8 unresolved discussions.


README.md, line 44 [r1] (raw file):

Inside the wlpcfg project there is the <code>it.springboot.security.HelloControllerTest</code> that will test the /springbootwebsocket endpoint to ensure it is working.

For the complete feature documentation, see the <a href="http://spring.io/guides/gs/messaging-stomp-websocket/">Using WebSocket to build an interactive web application</a>

We shouldn't link to the Spring guides, instead we should link to guides about how to use Spring with Liberty as the given link does not explain how to write Spring apps to run on Liberty.


Comments from Reviewable

@katheris
Copy link
Contributor

Reviewed 5 of 8 files at r2.
Review status: 23 of 27 files reviewed at latest revision, 8 unresolved discussions.


Comments from Reviewable

@katheris
Copy link
Contributor

Reviewed 1 of 8 files at r2.
Review status: 24 of 27 files reviewed at latest revision, 8 unresolved discussions.


Comments from Reviewable

@katheris
Copy link
Contributor

Reviewed 1 of 8 files at r2.
Review status: 25 of 27 files reviewed at latest revision, 8 unresolved discussions.


Comments from Reviewable

@katheris
Copy link
Contributor

Reviewed 1 of 8 files at r2.
Review status: 26 of 27 files reviewed at latest revision, 8 unresolved discussions.


Comments from Reviewable

@katheris
Copy link
Contributor

Reviewed 1 of 27 files at r1.
Review status: all files reviewed at latest revision, 8 unresolved discussions.


Comments from Reviewable

@katheris
Copy link
Contributor

Review status: all files reviewed at latest revision, 8 unresolved discussions.


README.md, line 37 [r1] (raw file):

Previously, katheris (Katherine Stanley) wrote…

This is inconsistent with the other spring microservices that put the Application class in application.spring and then the tech specific ones in the application.spring.tech package.

OK

Comments from Reviewable

@katheris
Copy link
Contributor

katheris commented Jul 1, 2016

This will be pulled into a branch rather than accepting this pull request. I will close the request when we have the code in a branch.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants