8000 introduce plugins to load openapi specs by giograno · Pull Request #11497 · localstack/localstack · GitHub
[go: up one dir, main page]
Skip to content

introduce plugins to load openapi specs #11497

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Sep 14, 2024
Merged

introduce plugins to load openapi specs #11497

merged 6 commits into from
Sep 14, 2024

Conversation

giograno
Copy link
Member
@giograno giograno commented Sep 10, 2024 •
edited
Loading

Motivation

This PR is a small improvement over #11452.

Changes

  • Added a plux plugin which is responsible for loading a spec given a certain path;
  • The validator can work now on multiple specs. For instance, pro would have its spec and plugin and the validation will be performed against all the specs the plugin manager can find.

@giograno giograno added the semver: patch Non-breaking changes which can be included in patch releases label Sep 10, 2024
@giograno giograno requested a review from alexrashed September 10, 2024 19:44
@giograno giograno changed the title use importlib_resources to load the openapi spec add utility method to return OAS spec Sep 11, 2024
@giograno giograno marked this pull request as ready for review September 11, 2024 18:06
@giograno giograno requested a review from thrau as a code owner September 11, 2024 18:06
@github-actions GitHub Actions
Copy link
github-actions bot commented Sep 11, 2024 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 35m 51s ⏱️ -42s
3 431 tests +5  3 032 ✅ +5  399 💤 ±0  0 ❌ ±0 
3 433 runs  +5  3 032 ✅ +5  401 💤 ±0  0 ❌ ±0 

Results for commit 2bf9217. ± Comparison against base commit 68366d1.

This pull request removes 1 and adds 6 tests. Note that renamed tests count towards both. 
tests.aws.services.stepfunctions.v2.scenarios.test_base_scenarios.TestBaseScenarios ‑ test_parallel_state

tests.aws.services.lambda_.test_lambda_api.TestLambdaRecursion ‑ test_put_function_recursion_config_allow
tests.aws.services.lambda_.test_lambda_api.TestLambdaRecursion ‑ test_put_function_recursion_config_default_terminate
tests.aws.services.lambda_.test_lambda_api.TestLambdaRecursion ‑ test_put_function_recursion_config_invalid_value
tests.aws.services.stepfunctions.v2.scenarios.test_base_scenarios.TestBaseScenarios ‑ test_map_state_result_writer
tests.aws.services.stepfunctions.v2.scenarios.test_base_scenarios.TestBaseScenarios ‑ test_parallel_state[PARALLEL_STATE]
tests.aws.services.stepfunctions.v2.scenarios.test_base_scenarios.TestBaseScenarios ‑ test_parallel_state[PARALLEL_STATE_PARAMETERS]

♻️ This comment has been updated with latest results.

alexrashed
alexrashed reviewed Sep 12, 2024
View reviewed changes
Copy link
Member
@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

While the change itself isn't looking bad, if the whole purpose is the extensibility, I think we should try to define a cleaner way for the extension.
What's the plan for merging multiple specs in the future? We need a mechanism for handling multiple specs here in Community as well. Maybe it would make sense to have multiple providers (maybe implemented in plux) which can provide their specs?

@giograno giograno changed the title add utility method to return OAS spec introduce plugins to load openapi specs Sep 12, 2024
@github-actions GitHub Actions
Copy link
github-actions bot commented Sep 12, 2024 •
edited
Loading

S3 Image Test Results (AMD64 / ARM64)

  2 files  ±0    2 suites  ±0   3m 29s ⏱️ -13s
423 tests ±0  369 ✅ ±0   54 💤 ±0  0 ❌ ±0 
846 runs  ±0  738 ✅ ±0  108 💤 ±0  0 ❌ ±0 

Results for commit 2bf9217. ± Comparison against base commit 68366d1.

♻️ This comment has been updated with latest results.

@giograno
Improvements to load oas specs
b63ad9c 
	- use fixed files in importlib_resources
	- add a utility function we can patch in ext
@giograno giograno requested a review from alexrashed September 12, 2024 16:16
alexrashed
alexrashed reviewed Sep 13, 2024
View reviewed changes
Copy link
Member
@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

Nice to see that the OpenAPI validator gets pluggable! In general the code looks good, but I would like to discuss the runtime implications of the plugin mechanism, and had some other nitpicks :P Afterwards we're good to go!

localstack-core/localstack/aws/handlers/validation.py
Copy link
Member

Choose a reason for hiding this comment

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

In my opinion, this handler and the plugin definition deserves quite a lot more documentation. It's a cool new feature where only a very small amount of our contributors know about. Please make sure that everyone can take a look at this file and knows what it does, and how one can interact with it (enable it, disable it, register OpenAPI Spec plugins,...). ;)

Copy link
Member

Choose a reason for hiding this comment

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

i second alex' comment here and think it can do with even a bit more doc. specifically i think it needs a more concrete example on how to use it. where do i put the openapi.yml? how do i correctly instantiate a new plugin? For some inspiration, maybe the docs on the WebAppExtension can help.

thrau
thrau approved these changes Sep 14, 2024
View reviewed changes
Copy link
Member
@thrau thrau left a comment

Choose a reason for hiding this comment

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

Nice! LGTM 👍 just a minor nit regarding documentation

localstack-core/localstack/aws/handlers/validation.py
Copy link
Member

Choose a reason for hiding this comment

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

i second alex' comment here and think it can do with even a bit more doc. specifically i think it needs a more concrete example on how to use it. where do i put the openapi.yml? how do i correctly instantiate a new plugin? For some inspiration, maybe the docs on the WebAppExtension can help.

@giograno
minor optimization and some docs
f14d87e
@giograno
Switch to import from module path
2bf9217 
	Forgot we don't have importlib_resources in the runtime dependencies
	and the standard library does not handle namespace packages
	installed in editable mode
@giograno giograno merged commit b90cbb1 into master Sep 14, 2024
41 checks passed
@giograno giograno deleted the oas-plugin branch September 14, 2024 23:32
@giograno giograno mentioned this pull request Sep 18, 2024
Merged
@lizard-boy lizard-boy mentioned this pull request Sep 20, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexrashed alexrashed alexrashed left review comments

@thrau thrau thrau approved these changes

@viren-nadkarni viren-nadkarni Awaiting requested review from viren-nadkarni

Assignees
No one assigned
Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@giograno @alexrashed @thrau
0