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

Refactor role creation for upgrade command path #2795

Merged
merged 8 commits into from
Oct 31, 2024

Conversation

viniciusdc
Copy link
Contributor

@viniciusdc viniciusdc commented Oct 24, 2024

Reference Issues or PRs

closes #2766

What does this implement/fix?

This issue is a patching fix for the upgrade command present in the previous release, currently the upgrade logic when requesting the user to perform the role creation (more details see linked issue), assumes the presence of the role when assigning it to the legacy groups. However, this leads to errors when the role does not exist or is within Terraform if the user attempts to manually address the missing role to continue the upgrade.

This PRs includes a new section in the previous code logic to create the role, and to avoid conflicts with terraform, I adopted a "legacy" prefix to the role name with a befitting description for future reference when the amdins manages keycloak in the future.

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?

How to test this PR?

  • Perform any type of deployment for Nebari v2024.7.1, local is advised,
  • create some testing groups and users with such users assigned to some of these groups (one user and one group should suffice).
  • Launch one user instance and assert the folders are mounted;
  • Install this nebari version in dev mode and perform the upgrade; (You will need to create a tag locally for proper installation of nebari)
  • Assert the new role has been created in the jupyterhub client, and the previous groups all have the role assignment;
  • Perform the deployment and launch a user instance to validate the folder mounting is kept the same (consider logging out first to avoid issues with caching of user_Data)

Any other comments?

@viniciusdc
Copy link
Contributor Author

There are no tests for this yet, and I would like to have this tested to help us avoid this situation in the future. I hope we get #2780 merged before this to extend the testing suit.

@viniciusdc
Copy link
Contributor Author

I will be testing this during the afternoon

@dcmcand
Copy link
Contributor

dcmcand commented Oct 28, 2024

Let's disable the 2024.9.1 upgrade step as part of this @viniciusdc

@viniciusdc
Copy link
Contributor Author

viniciusdc commented Oct 29, 2024

It was tested on a local deployment, moving from 2024.7.1 directly to 2024.9.2 (will need to test again since this now changed to 2024.11.1):

  • Group created on the previous version:
    Captura de tela de 2024-10-28 10 29 23
  • After upgrade command:
    • legacy role is included to existing groups:
      Captura de tela de 2024-10-28 10 46 53
    • after deployment, both roles now exist (no terraform conflict)
      Captura de tela de 2024-10-28 10 57 00
    • previous behavior of mounted groups folders is preserved
      Captura de tela de 2024-10-28 10 57 21

@viniciusdc
Copy link
Contributor Author

This needs to be re-tested to ensure the upgrade will run now that we moved it to 2024.11.1

@viniciusdc
Copy link
Contributor Author

Re-tested with the recent changes:
Captura de tela de 2024-10-30 14 01 33
image

src/_nebari/upgrade.py Outdated Show resolved Hide resolved
Copy link
Member

@marcelovilla marcelovilla left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@viniciusdc
Copy link
Contributor Author

viniciusdc commented Oct 30, 2024

These tests are failing only for DO, and it might be related to its decommissioning process. So, we can disregard it for now, I've created a separated issue for this so that it does not become a blocker #2810

Copy link
Contributor

@dcmcand dcmcand left a comment

Choose a reason for hiding this comment

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

🚀

Thanks for this @viniciusdc

@viniciusdc viniciusdc merged commit 9b1310b into main Oct 31, 2024
24 of 28 checks passed
@viniciusdc viniciusdc deleted the 2766-hotfix-upgrade-command-keycloak-roles branch October 31, 2024 16:08
viniciusdc added a commit that referenced this pull request Nov 1, 2024
Co-authored-by: Adam Lewis <23342526+Adam-D-Lewis@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done 💪🏾
Development

Successfully merging this pull request may close these issues.

[BUG] - 2024.9.1 Upgrade Bug with Auto-Creation of Group Directory Role
4 participants