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

WebSocket Extensions #1934

Merged
merged 2 commits into from Sep 14, 2020

Conversation

dansiviter
Copy link
Contributor

@dansiviter dansiviter commented Jun 5, 2020

Few notes:

  • Extensions (such as org.glassfish.tyrus.ext.extension.deflate.PerMessageDeflateExtension) that are in archives that are not 'bean archives' are not added as CDI ignores them. Is there a mechanism to override this or an acceptable limitation forcing workarounds? I thought about using synthetic bean archives, but the classpath would still need to be scanned to do this and not aware of any mechanism for this.
  • The only build available with the fixes required in Tyrus is 2.0.0+ which also mandates jakarta.websocket.* packages which means quite a few more changes. No longer an issue,
  • The both Tyrus and WebSocket API are not at release versions yet. I'm assuming this would prevent merging. No longer an issue.

@romain-grecourt
Copy link
Contributor

/oca-checked

@romain-grecourt
Copy link
Contributor

/trigger

@spericas spericas self-requested a review June 8, 2020 13:22
@spericas spericas added the 2.x Issues for 2.x version branch label Jun 8, 2020
@spericas spericas added this to Normal priority in Backlog Jun 8, 2020
@spericas
Copy link
Member

spericas commented Jun 8, 2020

  • The both Tyrus and WebSocket API are not at release versions yet. I'm assuming this would prevent merging.

This is a problem. What do you think @barchetta?

@barchetta
Copy link
Member

  • The both Tyrus and WebSocket API are not at release versions yet. I'm assuming this would prevent merging.

This is a problem. What do you think @barchetta?

I agree that this PR should stay in WIP for now because:

  1. It moves to jakarta.websocket. Moving to jakarta packages should be a coordinated decision across the project.
  2. We prefer not to depend on milestone builds if at all possible.

So this PR might need to stay in the wings until we make some decisions concerning moving to Jakarta EE 9.

@barchetta barchetta added the dependencies Pull requests that update a dependency file label Jun 8, 2020
@dansiviter
Copy link
Contributor Author

It appears Checkstyle doesn't like the ordering too:

<module name="ImportOrder">
            <property name="groups" value="java, javax, io.helidon"/>
            ...

Would this have to change to "java, javax, jakarta, io.helidon" or come after io.helidon?

@romain-grecourt
Copy link
Contributor

Good question. This is debatable :D ; I'd say it should be before io.helidon

@spericas spericas added this to the 2.0.1 milestone Jun 19, 2020
@dansiviter dansiviter force-pushed the feature/WebsocketExtensions branch 2 times, most recently from e5126bd to 0ab85ae Compare June 25, 2020 10:46
@dansiviter
Copy link
Contributor Author

It appears 1.17 has both the changes I submitted and still using javax.websocket so I've updated the MR. I guess this won't be in time for v2 release though.

@dansiviter dansiviter marked this pull request as draft June 25, 2020 10:48
@dansiviter dansiviter changed the title WIP: WebSocket Extensions WebSocket Extensions Jun 25, 2020
@dansiviter dansiviter marked this pull request as ready for review June 25, 2020 10:50
@dansiviter
Copy link
Contributor Author

Any idea how to re-trigger CI?

@barchetta
Copy link
Member

/trigger

@dansiviter
Copy link
Contributor Author

/trigger

@dansiviter
Copy link
Contributor Author

I'm hoping this will be merged soon but the CI never seems to trigger when I push. I tried using /trigger but that didn't seem to work.

@romain-grecourt
Copy link
Contributor

Basically accounts that do not have access to the repository are not "trusted" and their pull requests need to be manually triggered by "trusted" accounts.

@romain-grecourt
Copy link
Contributor

/trigger

@romain-grecourt
Copy link
Contributor

you have checkstyle errors, you can run the check locally: mvn validate -Pcheckstyle from ./webserver/tyrus/

@dansiviter
Copy link
Contributor Author

Thanks @romain-grecourt for the info. I've updated the MR with the checkstyle updates.

@romain-grecourt
Copy link
Contributor

/trigger

Copy link
Member

@spericas spericas left a comment

Choose a reason for hiding this comment

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

LGTM

@romain-grecourt
Copy link
Contributor

@dansiviter we need to do some checks for the Tyrus version change before we merge this.

@barchetta barchetta self-requested a review September 14, 2020 22:10
@barchetta barchetta modified the milestones: 2.0.1, 2.1.0 Sep 14, 2020
@barchetta
Copy link
Member

@dansiviter thanks for the contribution and apologies this took so long to merge!

@barchetta barchetta merged commit 701a01c into helidon-io:master Sep 14, 2020
Backlog automation moved this from Normal priority to Closed Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.x Issues for 2.x version branch dependencies Pull requests that update a dependency file
Projects
Backlog
  
Closed
Development

Successfully merging this pull request may close these issues.

None yet

4 participants