-
Notifications
You must be signed in to change notification settings - Fork 168
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.
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
Make the service path support variable depths #70
Conversation
I tested this in a private build and I'm not sure if it is actually working correctly. I'll try pushing a PR out later with fixes to the regular expressions. At the moment, I get a validation error. |
@Joholland What's the validation error? We're using the following build and so far we've not seen any issues https://github.com/splunkcloud/chamber/releases/tag/temp Update: Nevermind, I see the error (we're only use path based internally hence not hitting the issue) |
This looks promising. I'll try to fix the conflicts |
Just to put in my two cents -- we'd love to use |
I've merged this code into the latest master and built the binary. You can find it here https://github.com/people-ai/chamber/releases |
Thanks @vood -- it would be best if we could get the original maintainers of the code to merge it in though! |
Of course. My build is just a temporary workaround
…On Wed, Apr 25, 2018, 7:23 AM Joe Wilner ***@***.***> wrote:
Thanks @vood <https://github.com/vood> -- it would be best if we could
get the original maintainers of the code to merge it in though!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#70 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAZqTN_vVTosb6tEc-sYCkkt6vgXaqt1ks5tsIbEgaJpZM4Rkg4P>
.
|
@dfuentes any chance you could comment on whether or not there's a future for this PR? |
Sorry, I've been pretty busy with other projects. I will give this a look over today and see what we can do. |
In order to get this merged, I think we need some more validation. For example, I tried this build out and was able to get things in a pretty confusing state: $ export CHAMBER_NO_PATHS=1
$ chamber write test/with/mixed key val
Error: ValidationException: Parameter name must be a fully qualified name.
status code: 400, request id: e21e2ecf-7340-47d0-b885-b00f5ec1cc28
exit status 1 $ export CHAMBER_NO_PATHS=1
$ chamber write /test/with/mixed key val
$ chamber list /test/with/mixed
Key Version LastModified User Because the path based api doesn't play well with the older API, I suggest updating the validation such that service names may not contain forward slashes when |
We need this too , chamber is currently not usable. |
If it helps folks, I wrote something that doesn't provide all the perks of this project but is less opinionated about namespaces and keys: https://github.com/energyhub/secretly (all it does is aim to have a nice API for dumping values from your param store into your container) |
I'd say "chamber is currently not usable" is a bit of hyperbole. I'm not opposed to having this feature, but this PR in it's current state is not something I'm comfortable merging. If the validation is fixed up and the edge cases between paths/non-paths uses are fixed, then we can get it in. |
I've just discovered that chamber doesn't support this which makes it unusable for our use-case too. Looks like I shall have to use raw |
+1, this is highly anticipated feature for us. |
Thank you! This is exactly what I needed. |
It appears that #118 has built upon this PR and implemented this feature, so this specific PR can be closed. I've successfully tested variable-length service paths with the latest version of Chamber. Thanks, everyone! |
This PR makes it possible to have the service have a variable depth for the service (
<path>/<service>
). The default "." separator is also supportede.g
Happy to write some tests if needed but wanted to guage the desire to support this before spending too much time on it.