ui: [BUGFIX] Ensure in-folder KVs are created in the correct folder #10569
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🍒 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/409091. |
johncowen
added a commit
that referenced
this pull request
Jul 15, 2021
…10569) When clicking to create a KV within folder name, would would be viewing a form that was a form for creating a KV in the root, which when the user clicked to save, saved the KV in the root. For the moment at least I've removed the code that strips double slashes, and whilst this isn't ideal, it looks like we've picked up one of those bugs that turns into a 'feature', and completely reworking KV to not rely on the double slashes is not really an option right now.
|
🍒 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/410034. |
|
🍒✅ Cherry pick of commit b256313 onto |
hc-github-team-consul-core
pushed a commit
that referenced
this pull request
Jul 15, 2021
…10569) When clicking to create a KV within folder name, would would be viewing a form that was a form for creating a KV in the root, which when the user clicked to save, saved the KV in the root. For the moment at least I've removed the code that strips double slashes, and whilst this isn't ideal, it looks like we've picked up one of those bugs that turns into a 'feature', and completely reworking KV to not rely on the double slashes is not really an option right now.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In #10212 we made a fairly significant change to how our routing for namespaces works, with an eye to supporting a future feature of the UI (more details on that PR)
We based the majority of that work on code already existing in the ember framework but noticed that our app would slightly inconsistently use the some of the code in here, which meant it was possible to set empty URL segments in URLs for example
/kv/folder-name//create(the URL for creating a new KV within thefolder-namefolder).Ember seems to generally strips these, and whilst I've not looked too far into it as yet, this wasn't happening previous to the above PR.
https://github.com/emberjs/ember.js/blob/297ab0e771409d61dcb014a9a40144cd7fdd5ee8/packages/%40ember/-internals/routing/lib/location/history_location.ts#L145
With the addition of the above PR, the double slash stripping began to work, without us noticing! So when clicking to create a KV within folder name, would would be viewing a form that was a form for creating a KV in the root, which when the user clicked to save, saved the KV in the root.
For the moment at least I've removed the code that strips double slashes, and whilst this isn't ideal, it looks like we've picked up one of those bugs that turns into a 'feature', and completely reworking KV to not rely on the double slashes is not really an option right now.
This fixes the problem. Typically it doesn't look like we had a test here to cover this exact pattern, so I've added an additional line to our tests that fails before this bug was fixed.
Fixes #10502