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

RestClientProcessor - replace generated bean classes with BeanRegistrar #691

Merged
merged 2 commits into from
Feb 1, 2019
Merged

RestClientProcessor - replace generated bean classes with BeanRegistrar #691

merged 2 commits into from
Feb 1, 2019

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Jan 30, 2019

@mkouba mkouba requested a review from gsmet January 30, 2019 21:12
mkouba
mkouba commented Jan 30, 2019
View reviewed changes
.../rest-client/deployment/src/main/java/org/jboss/shamrock/restclient/RestClientProcessor.java Outdated
public void register(RegistrationContext registrationContext) {
for (Map.Entry<DotName, ClassInfo> entry : interfaces.entrySet()) {
BeanConfigurator<Object> configurator = registrationContext.configure(RestClientBase.class);
configurator.types(Type.create(entry.getValue().name(), Kind.CLASS));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should probably add all superinterfaces too.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason you didn't do it right away?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was too tired ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, in fact the spec does not define this and SmallRye only adds the rest interface type so I think we should align with SmallRye. And if possible switch to SmallRye ASAP.

@gsmet
Copy link
Member

gsmet commented Jan 30, 2019 •
edited
Loading

Looks like the tests are failing:

2019-01-30T21:44:54.0771729Z Caused by: java.lang.ClassNotFoundException: org.jboss.shamrock.restclient.runtime.RestClientBase_ClientProxy
2019-01-30T21:44:54.0771816Z 	at java.net.URLClassLoader.findClass(URLClassLoader.java:382)
2019-01-30T21:44:54.0794023Z 	at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
2019-01-30T21:44:54.0794265Z 	at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:349)
2019-01-30T21:44:54.0794774Z 	at java.lang.ClassLoader.loadClass(ClassLoader.java:357)

gsmet
gsmet approved these changes Jan 31, 2019
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I confirm it fixes my issue.

There is an unrelated test failure due to the new 2nd level cache code.

@gsmet gsmet mentioned this pull request Jan 31, 2019
Merged
@mkouba
Copy link
Contributor Author

mkouba commented Feb 1, 2019

@wburns Do you have any idea what's going on? Oops, shit happened, No boot for you! -> https://dev.azure.com/protean-ci/Shamrock/_build/results?buildId=1462

@gsmet
Copy link
Member

gsmet commented Feb 1, 2019

@mkouba they are on it, I reported it earlier today. Sanne commented the assertion for now so the tests should pass.

Could you rebase on top of master (David just merged the config PR)?

@mkouba mkouba force-pushed the issue-690-mp-rest-bean-registrar branch from ef87574 to 151464c Compare February 1, 2019 10:17
@mkouba
Copy link
Contributor Author

mkouba commented Feb 1, 2019

@gsmet rebased. Also @michalszynkiewicz promised to look at our MP Rest Client integration and explore the possibilities to switch to SmallRye (in fact, I have no idea why it isn't used yet).

@gsmet
Copy link
Member

gsmet commented Feb 1, 2019

Also @michalszynkiewicz promised to look at our MP Rest Client integration

OK, that's nice, we have a lot of code in this extension and it would be nice to trim it down.

@mkouba
Copy link
Contributor Author

mkouba commented Feb 1, 2019

OK, that's nice, we have a lot of code in this extension...

And most of the code is copied from the first version of SmallRye rest client ;-)

@mkouba mkouba merged commit a0edcf0 into quarkusio:master Feb 1, 2019
@gsmet gsmet added this to the 0.8.0 milestone Feb 1, 2019
maxandersen added a commit to maxandersen/quarkus that referenced this pull request Nov 5, 2022
@maxandersen
chore: clean up test setup and make it run in eclipse out-of-box (qua…
2343f6b 
…rkusio#691)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gsmet gsmet gsmet approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.8.0
Development

Successfully merging this pull request may close these issues.

RestClientProcessor - replace generated $$RestClientProxy classes with BeanRegistrar
2 participants
@mkouba @gsmet