Skip to content

Conversation

soapy1
Copy link
Contributor

@soapy1 soapy1 commented Jan 6, 2025

Reference Issues or PRs

fixes #2760

  • Runs conda-store integration tests as part of test_local_integration
  • Adds the option to specify a --group flag when adding a keycloak user

To create a keycloak user with access to a particular group, run the command

nebari keycloak adduser --user username password --config nebari-config.yaml --groups mygroup

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): CI change

@soapy1 soapy1 force-pushed the conda-store-black-box-testing branch 11 times, most recently from 8bb4c2a to 814d7e8 Compare January 7, 2025 22:09
@soapy1
Copy link
Contributor Author

soapy1 commented Jan 8, 2025

depends on conda-incubator/conda-store#1040

@viniciusdc
Copy link
Contributor

Since these tests depend on a live cluster, I suggest extending the already existing local integration tests with this extra check https://github.com/nebari-dev/nebari/blob/main/.github/workflows/test_local_integration.yaml -- since it uses CiRun under the hood you should not be affected by any flakiness of the GH runner in case of constrained resources.

@soapy1 soapy1 force-pushed the conda-store-black-box-testing branch 15 times, most recently from 8d941b5 to c108c2f Compare January 16, 2025 21:52
@soapy1 soapy1 force-pushed the conda-store-black-box-testing branch 2 times, most recently from 8138e62 to b1079d1 Compare February 12, 2025 21:07
@soapy1 soapy1 force-pushed the conda-store-black-box-testing branch 2 times, most recently from 7fbbdf9 to 9652b91 Compare February 13, 2025 16:30
@soapy1 soapy1 force-pushed the conda-store-black-box-testing branch from 9652b91 to a4e24a5 Compare February 13, 2025 16:30
@soapy1 soapy1 marked this pull request as ready for review February 13, 2025 16:30
@soapy1 soapy1 requested a review from a team as a code owner February 13, 2025 16:30
@soapy1 soapy1 requested review from dcmcand and removed request for a team February 13, 2025 16:30
@@ -29,7 +29,10 @@ def do_keycloak(config: schema.Main, *args):

username = args[1]
password = args[2] if len(args) >= 3 else None
create_user(keycloak_admin, username, password, domain=config.domain)
groups = args[3] if len(args) >= 4 else None
Copy link
Contributor Author

@soapy1 soapy1 Feb 13, 2025

Choose a reason for hiding this comment

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

I'm not a fan of this approach to passing arguments around but did this to follow the pattern already defined in this part of the code.
I would be happy to refactor this module to make it a more developer friendly if that's something the nebari team is into.
Could also split that into a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

why don't you split that into a different pr. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this functionality is also implemented in #2917. Will rebase my changes when it get's merged.

@soapy1 soapy1 force-pushed the conda-store-black-box-testing branch from 21578d2 to a17192b Compare February 13, 2025 19:03
@soapy1 soapy1 force-pushed the conda-store-black-box-testing branch from a17192b to 43a603c Compare February 13, 2025 19:04
@dcmcand dcmcand added the status: approved 💪🏾 This PR has been reviewed and approved for merge label Feb 20, 2025
@viniciusdc
Copy link
Contributor

viniciusdc commented Feb 25, 2025

@soapy1 this requires the nebari keycloak command update to work, right? If that's the case, @Adam-D-Lewis do you mind splitting that ENH into another PR to decouple from the service-account one? We can also keep it in here as well

@Adam-D-Lewis
Copy link
Member

@soapy1 this requires the nebari keycloak command update to work, right? If that's the case, @Adam-D-Lewis do you mind splitting that ENH into another PR to decouple from the service-account one? We can also keep it in here as well

Yeah, I can split that out into a separate PR today.

@Adam-D-Lewis
Copy link
Member

Here's the keycloak cli refactor PR - #2968

@dcmcand
Copy link
Contributor

dcmcand commented Mar 3, 2025

@soapy1 please let me know when this is ready to go so we can get it merged

@dcmcand dcmcand mentioned this pull request Mar 3, 2025
10 tasks
@marcelovilla
Copy link
Member

This looks great @soapy1 !

In the spirit of achieving testing parity, I think it would be worthwhile to also run these tests on cloud deployments. That's beyond the scope of this PR but something to keep in mind when we start running other test suites in cloud deployments too.

@dcmcand dcmcand self-assigned this Mar 6, 2025
@dcmcand dcmcand moved this from New 🚦 to In review/QA 👀 in 🪴 Nebari Project Management Mar 6, 2025
@marcelovilla marcelovilla added this to the 2025.4.1 milestone Mar 17, 2025
@marcelovilla marcelovilla merged commit 0a932d4 into nebari-dev:main Mar 20, 2025
13 checks passed
@github-project-automation github-project-automation bot moved this from In review/QA 👀 to Done 💪🏾 in 🪴 Nebari Project Management Mar 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: approved 💪🏾 This PR has been reviewed and approved for merge
Projects
Status: Done 💪🏾
Development

Successfully merging this pull request may close these issues.

[ENH] - add conda-store black-box testing to ci
6 participants