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

feat: fastapi schema-registry module with DI and E2E tests #995

Conversation

nosahama
Copy link
Contributor

@nosahama nosahama commented Nov 27, 2024

About this change - What it does

Some changes based off the work from the jjaakola-aiven-fastapi branch, majorly:

  • integrations tests which are failing need to be checked and resolved
    • some of the tests can be moved to the e2e folder
    • it'd be nice to stop spinning up all these kafka processes
    • it'd be nice to move this into the docker tests environment
    • i am not making any changes to the integration tests setup, so it's clear that we aren't changing logic that should verify the operability of our services
    • the integration tests fail now with issues spinning up kafka, so we can check that now or after we merge this work
  • unit tests now run within a docker compose environment
  • e2e tests were introduced, which also run within a docker compose environment
    • the plan is to extract some test out of the integration tests folder slowly into the e2e tests folder
    • e2e tests run against the karapace docker compose services: i.e. schema-registry/rest rather than local processes
  • karapace schema registry is now it's own module which houses all the FastAPI related things
  • karapace rest proxy still runs on rapu
  • we add dependency injection to the project
    • wiring the dependencies for both sr and rest
    • config is loaded on app startup
    • dependencies are injected into route handlers and other modules
    • modules with dependencies are wired on app startup
  • config is now based on pydantic-settings
    • settings are loaded via an .env file
    • base config is added as a yaml and packaged with the built python library
    • base config loads the reference to the .env file needed to populate the rest of the config
    • we can override the settings values directly via environment variables or via a helper method on the settings object itself
    • previously some other values were set, i.e. sentry config was added, we need to revisit this.
  • commits show progress
  • initial draft PR schema_registry: standalone module for SR with basic dependency injection #994 was rushed and contained more changes, this is a split out of that PR which will be closed at some point
  • we would need to check the coverage mechanism now with the new module and split

@nosahama nosahama requested review from a team as code owners November 27, 2024 10:59
@nosahama nosahama marked this pull request as draft November 27, 2024 12:18
@nosahama nosahama marked this pull request as ready for review November 27, 2024 12:19
@nosahama nosahama force-pushed the nosahama/fastapi-sr-module-with-DI-e2e-tests branch 5 times, most recently from 74c54af to d336032 Compare November 27, 2024 12:39
bin/smoke-test-registry.sh Outdated Show resolved Hide resolved
@nosahama nosahama force-pushed the nosahama/fastapi-sr-module-with-DI-e2e-tests branch from d336032 to f52fe6c Compare November 27, 2024 12:52
jjaakola-aiven and others added 10 commits December 2, 2024 10:51
- we create a standalone module for SR related components

- we use DI to wire together the SR dependencies

- we move the routers to own folder

- we move the config initialization to DI and app startup
- we add a docker compose service which would act like a CLI

- we add a dev Dockerfile

- we drop python3.9 from the tests matrix as pydantic-settings has issues with python3.9

- we add .env files for both registry and rest
- we replace use the karapace container to load the default config
@nosahama nosahama force-pushed the nosahama/fastapi-sr-module-with-DI-e2e-tests branch from f52fe6c to 6b3523a Compare December 2, 2024 19:31
@nosahama nosahama marked this pull request as draft December 3, 2024 12:12
.github/workflows/tests.yml Outdated Show resolved Hide resolved
src/schema_registry/user.py Outdated Show resolved Hide resolved
@Aiven-Open Aiven-Open deleted a comment from swarmia bot Dec 4, 2024
@nosahama nosahama force-pushed the nosahama/fastapi-sr-module-with-DI-e2e-tests branch 2 times, most recently from ca5d5e3 to 664a637 Compare December 4, 2024 15:49
@nosahama nosahama force-pushed the nosahama/fastapi-sr-module-with-DI-e2e-tests branch 12 times, most recently from e04c86a to 10878e7 Compare December 5, 2024 15:16
Copy link

github-actions bot commented Dec 5, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  src/karapace
  auth.py 214, 226, 305
  config.py 265, 273-290, 295-296
  container.py
  karapace_all.py 7-15, 20-35, 44-46
  logging_setup.py 6-46
  typing.py
  src/karapace/protobuf
  io.py 100
  src/schema_registry
  __main__.py 5-49
  container.py
  factory.py 5-59
  schema_registry_apis.py
  user.py 6-30
  src/schema_registry/http_handlers
  __init__.py 6-28
  src/schema_registry/middlewares
  __init__.py 6-33
  src/schema_registry/routers
  compatibility.py 6-37
  config.py 6-121
  errors.py
  health.py 6-10, 18-20, 38
  master_availability.py 6-13, 37-38
  metrics.py 6-24
  mode.py 6-47
  requests.py
  schemas.py 6-13, 24, 41, 70, 87
  setup.py 6-27
  subjects.py 6-17, 32, 47, 69, 93, 119, 134, 150, 175, 190
Project Total  

The report is truncated to 25 files out of 28. To see the full report, please visit the workflow summary page.

This report was generated by python-coverage-comment-action

@nosahama nosahama force-pushed the nosahama/fastapi-sr-module-with-DI-e2e-tests branch 6 times, most recently from 82396f1 to c3b0ec9 Compare December 6, 2024 12:22
@nosahama nosahama force-pushed the nosahama/fastapi-sr-module-with-DI-e2e-tests branch 3 times, most recently from 5115f7f to 364c47c Compare December 6, 2024 16:07
- we split out e2e tests into own folder

- e2e tests will run against the service in docker compose

- we start by splitting out the prometheus and kafka e2e tests which do not need local running services
@nosahama nosahama force-pushed the nosahama/fastapi-sr-module-with-DI-e2e-tests branch from 364c47c to af56c95 Compare December 6, 2024 16:08
@nosahama nosahama marked this pull request as ready for review December 9, 2024 09:26
Copy link
Contributor

@jjaakola-aiven jjaakola-aiven left a comment

Choose a reason for hiding this comment

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

LGTM, getting rather large and preferable to merge. Most integration tests pass, forwarding tests have some issues. Linting and other issues can be resolved in subsequent PRs.

@jjaakola-aiven jjaakola-aiven merged commit 4264981 into jjaakola-aiven-fastapi Dec 9, 2024
7 of 9 checks passed
@jjaakola-aiven jjaakola-aiven deleted the nosahama/fastapi-sr-module-with-DI-e2e-tests branch December 9, 2024 09:30
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.

2 participants