-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Swap route settings.configure-secret-backend for nested edit and index route under secret.configuration #27918
Conversation
CI Results: |
ui/app/routes/vault/cluster/secrets/backend/configuration/edit.js
Outdated
Show resolved
Hide resolved
Build Results: |
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.
Nice work, this change makes a lot of sense! I think there are some routing change process things to consider before we go ahead and ship this, so I'm holding off approval for now
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.
Beautiful! 🚢
Description
This PR moves the route
vault.cluster.settings.confgure-secret-backend
tovault.cluster.secrets.backend.configuration.edit
. It has also been renamed to "configuration/edit(js/hbs)".To keep the
vault.cluster.secrets.backend.configuration
route URL and behavior the same, I moved the code fromvault/cluster./secrets/backend/configuration
tovault/cluster/secrets/backend/configuration/index
. This way I could render the edit route without using {{outlet}}}. If there's another way to do this, I'm all ears.NO REDIRECT: We intentionally opted to not redirect if a user navigated to the old route. We could have either caught the old url in the page-error component, or kept the old route around and transitioned in a beforeModel hook. However, modifying a root component like page-error or keeping a legacy route around for a url that was not likely to be bookmarked seemed like too much code debt.
Why?
configure-secret-backend
was responsible for configuring secret-engines, but was a child ofvault.cluster.settings
and notvault.cluster.secrets.backend
. This made it difficult to access secret-engine things. You can see this struggle in how the route has to query all the secret engines and search for the type to see if the route should throw an error or proceed. However, if it was a child route ofvault.cluster.secrets.backend
it would already have access to the secret-engine model and would not need to query all them and then search through.mount-secret-backend
is under settings and they're related. Though, I personally don't thinkmount-secret-backend
should be under settings either 🥫🪱.Notes