-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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 Bugfix - kv trailing slashes #4373
Conversation
...therefore add a trailing slash to the API call
ui-v2/app/routes/dc/kv/index.js
Outdated
// add one to force a fake findBySlug | ||
if (!isFolder(key)) { | ||
key = key + '/'; | ||
} |
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.
Thinking about this some more - this solves the data fetching problem, but now you have 2 URLs that show the same thing which seems odd to me. What would you think about doing a this.replaceWith
in the beforeModel
hook that passes in a key with the slash on the end so that you always have a 1:1 mapping of URL to UI state?
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.
Took me a while to follow this, but this morning just had another look at that PR you sent over and I had a lightbulb moment! ah ha, thats what matt was talking about in that comment! 😄
I thought I couldn't add the trailing slash because it would redirect again, but of course we are in ember by this point so we can redirect to a trailing slash using the history API and it won't be 301'ed
Good call, will sort in a bit hopefully be here by the time you are in 👍
We now essentially do 2 redirects if you hit a `folder/` 1. If you visit `/ui/dc1/kv/folder/`, `consul` will redirect you to `/ui/dc1/kv/folder` 2. Once redirected to `/ui/dc1/kv/folder` via a 301, use ember/history API to redirect you back to `/ui/dc1/kv/folder/`. Bit long winded, but achieves what we want without having to get stuck into `consul` itself to remove the 301 for the UI
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.
Sorry for the delay! Looks good 👍
Fixes #4324
This PR addresses the fact that the UI has a redirect to remove trailing slashes off any URLs, which causes problems with KV 'folders'. So if you hand type a URL with a trailing slash it will redirect you to possibly a inaccessible URL (whereas if you are navigating there using the UI/javascript, it won't).
It goes by the basic premise that if you request the
index
orfolder
route you must be asking for a 'folder' therefore if the URL/Keyname has no trailing slash, then add one when calling the API.Also, I made sure I added a failing test as a separate first commit on this branch instead of bundling it up with the fix, so if you want see it breaking easily you can checkout one commit back and run
make test-view
, filtering by 'trailing' will take you to the broken test, re-checking out this branch will fix it. Will try and make this easier to do this moving forwards.