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

SSO, test additions, database work, ASGI middleware, and more #58

Merged
merged 82 commits into from
May 14, 2024

Conversation

aholmes
Copy link
Member

@aholmes aholmes commented May 3, 2024

This massive PR contains the work required to finish the CAP migration. That includes components for SSO, database migrations, ASGI, SSM, users/roles, sessions, integration between Flask and Connexion, various middlewares, and so on.

Dan and I will go over this PR in person and review the dark corners.

- Also reorganized Feature Flag components to fit the structure of the Identity tables.
src/identity/BL_Python/identity/SAML2/__init__.py Dismissed Show dismissed Hide dismissed
src/identity/BL_Python/identity/SAML2/__init__.py Dismissed Show dismissed Hide dismissed
src/identity/BL_Python/identity/SAML2/__init__.py Dismissed Show dismissed Hide dismissed
@dan-knight
Copy link

dan-knight commented May 13, 2024

Based on our discussion/review in person this looks good to me. A few notes:

  • I like the move to Config classes. We already see benefits when setting up the test environment(s).
  • A lot of this functionality is consistent with CAP (if not directly lifted from CAP).
  • We may revisit feature flags in the future. The tests look good to me, but we're not yet using the feature as extensively as we likely will in the future. So far, we're focusing on storing feature flags in the database. I wonder if we might need to extend this in future to allow other methods of handling feature configuration. Way outside the scope of this PR.

dan-knight
dan-knight previously approved these changes May 13, 2024
Copy link

@dan-knight dan-knight left a comment

Choose a reason for hiding this comment

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

Approving with Makefile updates for MacOS. make dev runs correctly for me, creating the virtual environment in the correct place with the correct Python 3.10.x version. make does not automatically find the target, but I think it's reasonable to just run make dev for now.

@aholmes
Copy link
Member Author

aholmes commented May 14, 2024

  • A lot of this functionality is consistent with CAP (if not directly lifted from CAP).

Much is lifted from CAP, but has either been modified to fit into BLP or to allow more generic uses.

  • We may revisit feature flags in the future. The tests look good to me, but we're not yet using the feature as extensively as we likely will in the future. So far, we're focusing on storing feature flags in the database. I wonder if we might need to extend this in future to allow other methods of handling feature configuration. Way outside the scope of this PR.

Definitely. The base classes are extendable, so it should not be difficult to at least get started with this.

@aholmes
Copy link
Member Author

aholmes commented May 14, 2024

Approving with Makefile updates for MacOS. make dev runs correctly for me, creating the virtual environment in the correct place with the correct Python 3.10.x version. make does not automatically find the target, but I think it's reasonable to just run make dev for now.

Cool. I still need to figure out some problem that doubled CI/CD runtimes, but I'm glad this works now.

This was caused by `PYTHON_VERSION` being, e.g., `3.10.14` while the venv
directory is named `python3.10`. Some checks to determine whether to
rebuild check whether a path under `.venv/lib/$PYTHON_VERSION/..`
exists, and those paths never exist because the variable value is not
what was expected. Instead, we now use `pip list`. This chnage required
altering when the venv is activated.
@aholmes aholmes merged commit 11f1e95 into main May 14, 2024
12 checks passed
@aholmes aholmes deleted the aholmes-identity-sso branch May 14, 2024 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants