Skip to content
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

Merged
merged 10 commits into from
Aug 1, 2024

Conversation

Monkeychip
Copy link
Contributor

@Monkeychip Monkeychip commented Jul 30, 2024

  • Enterprise test pass

Description

This PR moves the route vault.cluster.settings.confgure-secret-backend to vault.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 from vault/cluster./secrets/backend/configuration to vault/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?

  • The route FKA configure-secret-backend was responsible for configuring secret-engines, but was a child of vault.cluster.settings and not vault.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 of vault.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.
  • IMO, it makes sense here (the old path override also seems to agree). I'm actually not sure why it was under settings at all other than mount-secret-backend is under settings and they're related. Though, I personally don't think mount-secret-backend should be under settings either 🥫🪱.
  • This route is currently only used for two secret-engines: aws and ssh. It will be used for more and only get more complicated.

Notes

  • I ONLY moved files in this PR. There is sooo much more I could do (glimmerize, refactor methods, address current bugs). But because this is a significant change (anything routing always is), I wanted to make it clear what was actually happening. Those TODOs are coming, but not in this PR.
  • TLDR: I don't have to merge this. I can continue my WIF work without these changes, but it started to feel like I was pilling fresh food onto a dirty plate. I don't have the time to clean the whole kitchen, but maybe tidy up a bit?

@github-actions github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Jul 30, 2024
Copy link

github-actions bot commented Jul 30, 2024

CI Results:
All Go tests succeeded! ✅

@Monkeychip Monkeychip added this to the 1.18.0-rc milestone Jul 30, 2024
@Monkeychip Monkeychip added the ui label Jul 30, 2024
@Monkeychip Monkeychip marked this pull request as ready for review July 31, 2024 15:36
@Monkeychip Monkeychip requested a review from a team as a code owner July 31, 2024 15:36
Copy link

Build Results:
All builds succeeded! ✅

Copy link
Contributor

@hashishaw hashishaw left a 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

Copy link
Contributor

@hashishaw hashishaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful! 🚢

@Monkeychip Monkeychip merged commit 01709e9 into main Aug 1, 2024
31 checks passed
@Monkeychip Monkeychip deleted the ui/VAULT-28468/router-changes branch August 1, 2024 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants