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

Add middleware to prefix JupyterHub navbar items with /hub. #2360

Merged
merged 3 commits into from
Apr 4, 2024

Conversation

marcelovilla
Copy link
Member

Reference Issues or PRs

Closes #2359

What does this implement/fix?

Put a x in the boxes that apply

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds a feature)
  • Breaking change (fix or feature that would cause existing features not to work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • Other (please describe):

Testing

  • Did you test the pull request locally?
  • Did you add new tests?

Any other comments?

@marcelovilla marcelovilla requested a review from viniciusdc March 26, 2024 22:50
@marcelovilla
Copy link
Member Author

I ran into an issue when trying to apply these changes in an existing deployment, having Terraform yield the following error:

[terraform]: │ Error: There was a field manager conflict when trying to apply the manifest for "dev/jupyterhub"
[terraform]: │
[terraform]: │   with module.jupyterhub.kubernetes_manifest.jupyterhub,
[terraform]: │   on modules/kubernetes/services/jupyterhub/main.tf line 214, in resource "kubernetes_manifest" "jupyterhub":
[terraform]: │  214: resource "kubernetes_manifest" "jupyterhub" {
[terraform]: │
[terraform]: │ The API returned the following conflict: "Apply failed with 1 conflict:
[terraform]: │ conflict with \"kubectl-edit\" using traefik.containo.us/v1alpha1:
[terraform]: │ .spec.routes"
[terraform]: │
[terraform]: │ You can override this conflict by setting "force_conflicts" to true in the
[terraform]: │ "field_manager" block.

I was able to manually delete the manifest in the k8s cluster and then the nebari deploy worked.

@viniciusdc should I add the field_manager block as suggested or do you think there's a better approach?

@viniciusdc
Copy link
Contributor

Hi @marcelovilla, have you tried doing a fresh install of the current version and applying the changes from the branch over? i remember we did some tinkering with the k8s config manually, I wonder if the message above comes from that

@viniciusdc
Copy link
Contributor

I had some issues running nebari today, so I will be able to test this only tomorrow morning, but in the mean time, as part of out testing overhaul mindset, it might be interesting to add a cypress test for this:
visit admin -> visit any other page
if the test passes then this is fixed

@marcelovilla
Copy link
Member Author

@viniciusdc I did and it works just fine. You are right, the error message was because we were manually editing the configuration on that deployment.

Copy link
Contributor

@viniciusdc viniciusdc left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the amazing work @marcelovilla -- as soon as the tests finishes we can merge this

@viniciusdc viniciusdc added this to the Next release milestone Apr 4, 2024
@viniciusdc viniciusdc added type: bug 🐛 Something isn't working area: user experience 👩🏻‍💻 status: approved 💪🏾 This PR has been reviewed and approved for merge labels Apr 4, 2024
@marcelovilla
Copy link
Member Author

as part of out testing overhaul mindset, it might be interesting to add a cypress test for this: visit admin -> visit any other page if the test passes then this is fixed

I agree it might make sense to add some specific tests for this kind of workflows (or user journeys) but if it's ok with you, I would prefer doing so after we have finished the discussion on how to structure our tests in the future.

@marcelovilla marcelovilla merged commit 994b57b into develop Apr 4, 2024
26 checks passed
@marcelovilla marcelovilla deleted the jupyterhub-navbar-redirect branch April 4, 2024 22:46
@marcelovilla marcelovilla removed this from the Next release milestone Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: user experience 👩🏻‍💻 status: approved 💪🏾 This PR has been reviewed and approved for merge type: bug 🐛 Something isn't working
Projects
Development

Successfully merging this pull request may close these issues.

[BUG] - JupyterHub navbar items not redirecting correctly after visiting Admin Dashboard
2 participants