eclipse / kuksa.val

kuksa.val
Apache License 2.0
95 stars 51 forks source link

[databroker] optional dependency for TLS #635

Closed AkhilTThomas closed 1 year ago

AkhilTThomas commented 1 year ago

The current version of databroker always adds tonic::tls as a mandatory dependency. Can this be moved a feature based dependency ? since tls is anyways an optional feature

lukasmittag commented 1 year ago

could be optional I guess. But then we need to change it in the Dockerfile too, There we need it because otherwise tls won't work with the containers. and also we need to update the documentation. So could e an optional dependency, but I think it would be simpler to keep it mandatory. But I'm open for arguments if there are some to make it optional.

AkhilTThomas commented 1 year ago

The reason was to get a leaner version of databroker for systems that don't support openssl. adding tls makes it pull lot more dependencies like rusttls, hyper-tls. this can be avoided if tls is not used.

SebastianSchildt commented 1 year ago

I think it might make sense, we should however for "default" builds and things we release for Linux always default to include TLS, as this is the right thing™ to do. So in terms of Cargo would be good, if we could express it as antifeature "no-tls" that needs to be enabled explicitly, if that is possible.

SebastianSchildt commented 1 year ago

maybe this can be some inspiration https://github.com/seanmonstar/reqwest/pull/375

AkhilTThomas commented 1 year ago

Yes, something like Cargo.toml

default = ["tls"]  
notls = [ tonic/transport , tonic/channel, tonic/prost]
tls   = [ tonic/transport , tonic/channel, tonic/prost, tonic/tls ]

and then wrap everything with tls in #[cfg(feature = "tls")]

AkhilTThomas commented 1 year ago

I have created a PR #638. @SebastianSchildt Can you review ?