-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
ui: Allow ${ } interpolation for UI Dashboard template URLs #11328
ui: Allow ${ } interpolation for UI Dashboard template URLs #11328
Conversation
🤔 This PR has changes in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @radiantly looks fantastic! Will take it for a spin (brb), but looks like we can merge this 🎉
- `{{Service.Namespace}}` or `${Service.Namespace}` - Replaced with the current service's namespace or empty if namespaces are not enabled. | ||
- `{{Datacenter}}` or `${Datacenter}` - Replaced with the current service's datacenter. | ||
|
||
~> **Note:** The `${}` style interpolation is only available from Consul TBD. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkirschner-hashicorp Should we add here something about that folks should probably prefer the style ${}
to avoid any issues with helm? Or something to that effect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@johncowen: if we recommend ${ }
, the ${ }
option should be given first in the listing above, e.g.,:
`${Service.Name}` or `{{Service.Name}}` - Replaced with ...
Additionally, I agree that this sentence can be expanded to explain why ${ }
is preferred (in version in which it is available). Speaking of versions... would this be in version 1.11.0+?
@johncowen : I'm actually not very familiar with configuring consul-k8s
- are all things configurable for Consul also configurable through the Helm chart? (I don't see this option on the Helm docs page).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jkirschner-hashicorp ,
Some more info here #10510 (comment) and hashicorp/consul-helm#1006.
Maybe we should get a changelog added here and then we can either ask @radiantly here to amend the ordering or we can merge and decide on the docs things ourselves from there. Up to you, pinging @david-yu also to see if he can help any of us further, he might have some info on the helm question.
Thanks all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I'd be happy to amend the ordering and add a note on the style preference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@johncowen: once merged, would this be in the next 1.10 release (1.10.4)? Or 1.11.0+?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@radiantly : can you add a .changelog\11328.txt
file with the content below (describing the change)?
```release-note:improvement
config: <brief description of the improvement you made here>
```
I thought the process of writing a changelog entry was already in the contributor docs, but I can't seem to find it... so that's something I'll fix!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the changelog note and amended the docs - I hope it is fine?
Looks good to me @radiantly ! Thanks for sorting @jkirschner-hashicorp I'll wait until you are around, but I think we should merge as is and then we can discuss what needs to be in the docs with Helm folks here and tweak what needs to be "twoked" [tm] (yeah I'm doin' that thing 😆 ) Lemme know when you are around and we can push the button.
I was told by previous folks that this should go in a 1.11 release, but we aren't negatively affecting anyone on 1.10 here if we add it there, in fact we are making their lives better. The further back we can backport this the less folks are likely to get annoyed when they run into it. If it was me, I'd backport it as far as possible (within our support matrix) which means 1.8. To add this to 1.8 its the same fix but in a different place: consul/ui-v2/app/components/templated-anchor/index.js Lines 27 to 29 in eef56f2
so we could try and botport back to 1.9 and then manually make the same amend here onto 1.8 (or @radiantly could do the manual amend if he wanted to get more involved?). Oh, but yeah actually if we are botporting, we need to decide before that before pushing the button, shout me when you are around. (cc @evrowe, please pitch in if you have any opinion on if/how far back we go here) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small change and then we'll approve!
@radiantly Thank you for sticking with us during the review process and for being so responsive to our change requests! We appreciate you taking the time to make this contribution! |
@radiantly : fully approved and ready for you to click the merge button :) thanks for the contribution (and thorough tests)! |
Hey @radiantly Thanks for the contribution here very much appreciated. I'm not totally sure whether you will have perms to push the merge button or not 🤔 , but I might be wrong. If you can't, lemme know and I can push it from here for you. One last favour I have to ask if you have the time. Do you have any feedback for us on the work here for example either:
..or any other type of feedback of course! Thanks, and keep up the good work! John |
Hi, Yeah, I don't have permission to push, so you can go ahead and merge this pull request. As for feedback - I don't have anything specifically I'd like to comment on, but I'd really like to appreciate how well the original issue was laid out. It was quite detailed, and was a delight to fix (thanks @jkirschner-hashicorp). radiantly |
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/478677. |
🍒✅ Cherry pick of commit cd55c0c onto |
…nterpolation ui: Allow ${ } interpolation for UI Dashboard template URLs
🍒✅ Cherry pick of commit cd55c0c onto |
…nterpolation ui: Allow ${ } interpolation for UI Dashboard template URLs
ui: Revert #11328 allow-${}-style-interpolation due, to browser support
ui: Revert #11328 allow-${}-style-interpolation due, to browser support
ui: Revert #11328 allow-${}-style-interpolation due, to browser support
Fixes #11321