Put rvps dependencies behind rvps-builtin feature flag
#550
Conversation
Signed-off-by: Ubuntu <azureuser@tdx-base-test.3z0tm1yprcsefaq3x0wq2wcmyg.cx.internal.cloudapp.net>
cdrappi
force-pushed
the
rvps-builtin_feature_flag
branch
from
b4ab0fe
to
3d7465b
Compare
Signed-off-by: Ubuntu <azureuser@tdx-base-test.3z0tm1yprcsefaq3x0wq2wcmyg.cx.internal.cloudapp.net>
| #[cfg(not(feature = "rvps-builtin"))] | ||
| { | ||
| return "LocalFs".into() | ||
| } |
There was a problem hiding this comment.
unclear what the correct thing to do is here
There was a problem hiding this comment.
hmm I think the best solution here would be to get rid of these default functions entirely and change the config struct to have Optional fields. Then we can use unwrap_or to swap in the default values as needed (i.e. inside of the RVPS module). That's slightly more work but I think it will untangle the dependencies.
There was a problem hiding this comment.
The configuration part about RVPS seem not good here. If remote_addr is set, the store_type is actually not useful. It might be better to not have useless fields that can be set. Let me make a fix to cover this. I made a PR #553
There was a problem hiding this comment.
I would have settled for a simpler fix but #553 seems good at first glance.
| #[cfg(not(feature = "rvps-builtin"))] | ||
| { | ||
| return "LocalFs".into() | ||
| } |
There was a problem hiding this comment.
hmm I think the best solution here would be to get rid of these default functions entirely and change the config struct to have Optional fields. Then we can use unwrap_or to swap in the default values as needed (i.e. inside of the RVPS module). That's slightly more work but I think it will untangle the dependencies.
|
Thanks @cdrappi for raising this up. The current RVSP config is not implemented well as it does not have an explicit field to specify whether a local one or a remote one to use. Probably we need to tidy things up. |
First of all thank you for building and maintaining such a wonderful suite of libraries! CoCo has been immensely valuable for our team and we appreciate all of the work that the community has put into these projects
Problem
Currently if you run build without the
rvps-builtinfeature, it fails:cargo build --no-default-features --features restful-bin,rvps-grpcThis is because the
reference-value-provider-servicedependency is not included under this feature set (requiresrvps-builtin), yet it is used outside of that flagProposed solution
This change tucks usage of
rvpsbehind that flag, so the above built command will complete successfully