DROOLS-5351 - Allow to provide relative resource resolver for DMN runtime #2899
Conversation
|
Hi @mswiderski
not sure if I understood correctly, but the Wrt Jenkins execute full downstream build |
|
@tarilabs ok, if that is WB only then it might not be future proof approach :) would it be ok to make the
The main reason is that I already have the imported asset loaded so there is no need to resolve it again from class loader/url resource. |
|
allow me to double-check if my memory is serving me well @mswiderski about the |
|
@tarilabs any additional thoughts on this one? |
|
didnt' have time to go into this sorry. Jenkins execute full downstream build |
|
Jenkins test again |
|
fdb failures unrelated, but waiting the relaunch of the standard build |
|
@mswiderski I did some checks, and I believe this is fine as long as build becomes green (normal build, failures in fbd to me is unrelated). This is indeed used mainly by the WB when need to validate a DMN model importing a PMML one, see test: https://github.com/kiegroup/drools/blob/cf139d465922e998fe9322ed9d3e3d93c45cb5ee/kie-dmn/kie-dmn-validation/src/test/java/org/kie/dmn/validation/ValidatorImportTest.java#L214-L216 yet I believe your modification make sense overall, if the resolver is present that shall be given the priority. |
|
thanks @tarilabs for feedback. and what about exposing option to provide the resolver via DMNRuntimeBuilder? would this be possible... I guess a new jira for this is required and then will update commit message and move this to normal PR instead of draft. Just waiting on your comment about DMNRuntimeBuilder |
I believe I already answered about that in this https://github.com/kiegroup/drools/pull/2899#issuecomment-628668075 ? If that is "kinda pressing" we can include analogous facility to the
|
|
@tarilabs the thing is that I need to be able to reference some resources from outside and thus having an option to provide this to not be expected on class path is what I am after. I can make a small prototype for the builder to illustrate what I have in mind, wdyt? |
Absolutely. Please keep in mind however that My original idea is to supply or analogous. If you have a prototype to look at we can discuss! |
mswiderski
force-pushed
the
relative-resolver-change
branch
from
92ca7d6
to
fcf756c
Compare
mswiderski
force-pushed
the
relative-resolver-change
branch
from
fcf756c
to
65ff882
Compare
|
@tarilabs here you can find the changes that allow to used the resolver via the builder kiegroup@65ff882 |
|
@mswiderski thank you for the commit. As I tried to explain before in this https://github.com/kiegroup/drools/pull/2899#issuecomment-630698748 I believe the function for resolution shall be Let's make an example a "project" like the following: in this case the minimal guarantee (for Drools DMN) is that DMN files must have unique combination of namespace+name, but there is no "requirement" for the relative URI to be unique, that for each model could be like Would work from your side if we make the function: I can take care of this, once we agree it overall address your needs on your end (I believe it does, since you could always ignore the additional parameters of the function) |
|
@tarilabs fine with me, I made this to be aligned with the compile method of DMNCompilerImpl to avoid extra changes that I might not be aware of side effects. Though, what you presented makes sense to me and if that will make it more future proof then go ahead! thanks |
|
@mswiderski let me know if mswiderski#1 works from your side |
|
@tarilabs sure thing, on it directly, thanks |
|
@tarilabs works like a charm, shall I merge your PR and update this one? |
|
yes if you merge the mswiderski#1 it shall automatically reflect here |
|
(if you can, remember to merge-with-squash) |
|
will merge locally and squash so all work is in single commit ... and obviously create jira :) thanks @tarilabs |
|
to clarify: the reason I would opt for the merge-with-squash in the linked PR, it would just "append" a single commit here. To fully clarify, I'm personally not a fan of the original author always squashing the PR, as this causes "force push" that for long PR under review make reviewers loose track of it :) my2c |
mswiderski
changed the title
|
@tarilabs as you wish, PR merged into this one and jira created, title updated. All good? |
|
allow me today/tmr to append one simple test, the implementation looks fine for me |
|
@mswiderski please merge-with-squash this mswiderski#2 so to have test coverage and we should be good |
|
@tarilabs done, thanks! |
|
@mswiderski please also this: mswiderski#3 |
|
done |
|
Kudos, SonarCloud Quality Gate passed!
|
@tarilabs could you please have a look and let me know if this has any side effects? We would like to pass resources not from the class loader so using relativeResolver seems to work.
In general it makes sense to have the resolver checked first before going via the resource which seems to always be there as it is the dmn resource if I got it right.
If this could be the way to go maybe we could expose the relative resolver as well via DMNRuntimeBuilder...