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

MicroProfile starter integration #408

Closed
m0mus opened this issue Feb 14, 2019 · 17 comments
Closed

MicroProfile starter integration #408

m0mus opened this issue Feb 14, 2019 · 17 comments
Assignees
Milestone

Comments

@m0mus
Copy link
Contributor

m0mus commented Feb 14, 2019

The MP starter project includes a PR for Helidon support: eclipse/microprofile-starter#4

@m0mus m0mus assigned tjfontaine and tjquinno and unassigned tjfontaine and tjquinno Feb 14, 2019
@tjquinno
Copy link
Member

tjquinno commented Feb 14, 2019

Currently the PR is on hold, it says, until Helidon supports MP 1.2.

Our helidon.io site does not mention that we do support 1.2, and the only mention I'm aware of is the FAQ linked off our wiki on our GitHub site.

I talked to Joe about this and he is going to mention that we support 1.2 in the 1.0.0 release notes, so once that is published I'll update the PR. (We've supported 1.2 at least since 0.11.0 but because that claim is so well hidden I'm waiting to update the MP starter PR!)

@tjquinno
Copy link
Member

It turns out that Rudy (who opened the MP PR re: Helidon support) already updated that with a link to our release tag.

@tjquinno
Copy link
Member

I have been conversing with Rudy. He had earlier merged support for Helidon into the MP starter but withdrew it until we more noticeably advertised our MP 1.2 support.

With our 1.0.0 release he seems to be very willing to get Helidon support into the tool.

I have looked at the starter code and also at Rudy's earlier withdrawn commit. There are some things we will want to recommend are done differently to simplify the Helidon support.

At his request I will provide a zip file that works as if the starter had created it. My plan is to produce that zip by running a locally-updated version of the starter so I can make specific suggestions about how the starter can best be revised to support Helidon. It will be Rudy who eventually pushes the changes.

@tjquinno
Copy link
Member

tjquinno commented Feb 28, 2019

There are two implementation choices made in Helidon MP that are different from those made in other MP impls that affect starter support for Helidon:

  • @ClaimValue datatypes supported (Helidon requires for example JsonString and does not accept String which the other impls accept), and
  • Helidon MP requires @PermitAll on endpoint classes or methods if the application class has @LoginConfig.

Changes in the code generation to customize what is produced for Helidon allow the starter-generated Helidon code to build and run, and we could do that. It would, though, mean that Helidon would need a custom version of nearly every Java class template which would be awkward.

@tjquinno
Copy link
Member

For reference: to run the token test this works:

mvn test-compile exec:java -Dexec.mainClass="com.example.demo.JWTClient" -Dexec.classpathScope="test"

@tomas-langer
Copy link
Member

Issue for @ClaimValue using a different type:
eclipse/microprofile-starter#92

@tjquinno
Copy link
Member

tjquinno commented Mar 1, 2019

I had asked Rudy Debusscher about rough timelines for the release of the MP starter. His response:

We haven't fixed a date for this yet. If you look at our milestone 0.9, you see that we have also a few things which needs to be done before we can go final. I would say this would not happen before the end of March (somewhere in April is a good guess). That is my opinion, I'll put it on the agenda of next weeks meeting.

If we have any needed changes to Helidon done and released by late March at the latest we should have time to feed suggestions about Helidon support in the starter to Rudy so he can incorporate them before starter 1.0 comes out.

@tjquinno
Copy link
Member

tjquinno commented Mar 7, 2019

I have changes to the MP starter ready to send to the starter team, once a new release of Helidon includes recent changes by Tomas (PR #465 to resolve issue #455).

This issue eclipse/microprofile-starter#92 Tomas filed is still open. My proposed starter changes include a Helidon-specific template for the affected class that addresses this, at least temporarily. That Helidon-specific template can be removed later or left in place, depending on how that issue is eventually resolved.

We can work around this other spec issue Tomas raised eclipse/microprofile-jwt-auth#124 thanks to his recent merge.

@m0mus m0mus modified the milestones: 1.1, 1.0.1, 1.2 Mar 10, 2019
@tjquinno
Copy link
Member

tjquinno commented Mar 14, 2019

With the release minutes ago of Helidon 1.0.1 I have verified that my updated MP starter works with 1.0.1 (it does) and written to Rudy at the MP starter team, pointing him to my fork of the starter with the Helidon-related changes.

I expect he will create a new (or revise the earlier) Helidon support PR using the latest changes. I've asked him for a rough estimate when that'll happen.

I will leave this issue open until the Helidon support is merged into the starter master.

@tjquinno
Copy link
Member

I have heard back from Rudy, who looked at the changes and has informally given them the thumbs-up.

He asked me to create a PR which I have: eclipse/microprofile-starter#114

@tjquinno
Copy link
Member

tjquinno commented Mar 18, 2019

Oracle's requirements for submitting changes to open source projects not initiated by Oracle include adding the Oracle copyright notice to new or changed source files. . . . Also today I discovered that the plan is now to release MP starter next week.

Rudy is ready to submit the Helidon-related changes himself tomorrow (19 Mar) so I have asked him to do so, rather than wait until I get complete resolution with the legal folks (assuming it would relax the Oracle copyright reqs which I suspect is unlikely) and then hope we can do the appropriate internal approvals and reviews in time. Rudy will credit us in comments, but most importantly the upcoming release of the starter will support Helidon.

@tjquinno
Copy link
Member

tjquinno commented Mar 21, 2019

Rudy has indeed created a new PR for the Helidon support in the MP starter. He is waiting for a review from Emily Jiang.

In separate communication Rudy has said they are hoping to release starter 1.0 next week.

@tjquinno
Copy link
Member

tjquinno commented Mar 26, 2019

Rudy has merged in the changes to the MP starter to support Helidon.

In an email to me he referred to "quite some discussion" about whether he would be able to do this and therefore whether Helidon would be supported, but there is nothing in the issue, the PR, or the gitter discussion about this so I have asked him to let me know what the concerns were so we (the Helidon team), if we're able, can make future updates smoother.

I'll close this issue since the Helidon support is in now -- Rudy said the production site at https://start.microprofile.io should be updated by tomorrow (27 Mar 2019) -- but I will update this issue later if/when I hear back from Rudy about the concerns.

@tjquinno
Copy link
Member

tjquinno commented Mar 26, 2019

The updated starter is, in fact, now accessible at https://start.microprofile.io and the Helidon support is there! Select MP 1.2 and then choose Helidon from the list of servers, then click Download to generated and download the project.

@spericas
Copy link
Member

spericas commented Mar 26, 2019

Looks like its https://start.microprofile.io

@tjquinno
Copy link
Member

Yup. I had it right in yesterday's post but botched it in today's post (now corrected) while fixing a separate typo. Sheesh.

@tjquinno
Copy link
Member

tjquinno commented Mar 26, 2019

Sigh. The correct URL is https://start.microprofile.io

Apologies for the confusion. Not sure what's going on with my URLs.

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

No branches or pull requests

5 participants