Fix #518 Rely on configurers for Configuration classes instead of using #647
Conversation
|
ok to test |
|
Strange, the Consul test is passing for me locally. Let me figure out what is different here. |
|
A race for the configurers set on the component is my first theory. |
Much simpler: stale deployment modules in my local Maven repo Jenkins was right and I can reproduce. Looks like this change needs to wait for Camel 3.1.0 because https://issues.apache.org/jira/browse/CAMEL-14263 was not fixed for all components in 3.0.1. |
ppalaga
force-pushed
the
i518
branch
2 times, most recently
from
8d3a3b0
to
36102f2
Compare
|
36102f2 rebased and tested properly |
| "org.apache.camel.component.fhir.FhirHistoryEndpointConfiguration", | ||
| "org.apache.camel.component.fhir.FhirMetaEndpointConfiguration", | ||
| "org.apache.camel.component.fhir.FhirPatchEndpointConfiguration", | ||
| "org.apache.camel.component.fhir.FhirDeleteEndpointConfiguration", |
There was a problem hiding this comment.
would' be better to have a build item generated by the respective extensions ? developer writing custom camel component extension outside this repo could also leverage that build item if needed.
There was a problem hiding this comment.
No, I'd prefer a central location to see better what is whitelisted. I hope the list will gradually disappear.
There was a problem hiding this comment.
But external extension won't have a way to make it working
There was a problem hiding this comment.
True. I was thinking of creating a dedicated extension for this (let's call it "policy extension") that would only be pulled by our tests. This would allow us to enforce the policy for our extensions and let 3rd party extensions do what they want. WDYT?
There was a problem hiding this comment.
I really do not like to have to touch core to make an extension working so if it were me to decide, I'd go for a simple build item also, I do not know it it really make sense to put such enforcement
There was a problem hiding this comment.
I really do not like to have to touch core to make an extension working so if it were me to decide, I'd go for a simple build item.
I still think policy definitions are better to keep centrally, but OK, let me try to implement this with a build item.
also, I do not know it it really make sense to put such enforcement
Well, I think this is the most effective, scalable (and sustainable) way to reduce the use of reflection. I do not think it is possible to be able to keep control once we will have 100+ extensions.
| .collect(Collectors.toSet()); | ||
|
|
||
| if (!violations.isEmpty()) { | ||
| throw new IllegalStateException( |
There was a problem hiding this comment.
all the camel components have a property that can control if the generated configurer have to be used or not so as long as the property is there, I feel we should log a warning but I'd rather avoid to fail the build.
…of using reflection
|
c4c1663 introduces a build item as requested by @lburgazzoli |
| .collect(Collectors.toSet()); | ||
|
|
||
| if (!violations.isEmpty()) { | ||
| throw new IllegalStateException( |
There was a problem hiding this comment.
Should we really fail the build ?
There was a problem hiding this comment.
Yes, this is only with our tests. It won't happen at runtime (unless the user explictly depends on the new policy extension)
| "org.apache.camel.component.consul.ConsulConfiguration", | ||
| "org.apache.camel.component.consul.ConsulClientConfiguration", | ||
| "org.apache.camel.component.consul.health.ConsulHealthCheckRepositoryConfiguration", | ||
| "org.apache.camel.component.consul.cloud.ConsulServiceRegistryConfiguration"); |
There was a problem hiding this comment.
It does not change much from the previous implementation as when we'll remove i.e. PdfConfiguration from the pdf extension we have to remember to get to this artefact and change it.
I really don't get why this should not be side-by-side with the code declaring the need for the reflective classes.
There was a problem hiding this comment.
These are the anomalies we need to tackle. They will hopefully disappear soon. If some of these turns out to be more stable, the item can be moved to the respective extension.
reflection
Removal of NettyConfiguration, NettyServerBootstrapConfiguration, PdfConfiguration registration led to native failures so these need a separate investigation.