Skip to content

apigw parity work on Model resource#7828

Merged
bentsku merged 9 commits intomasterfrom
apigw-parity-work-models
Mar 16, 2023
Merged

apigw parity work on Model resource#7828
bentsku merged 9 commits intomasterfrom
apigw-parity-work-models

Conversation

@bentsku
Copy link
Contributor

@bentsku bentsku commented Mar 9, 2023 •
edited
Loading

Adjusting parity for Model resource.
I took the opportunity to move the Model resource out of moto and into our own store, as there was no dependency between any other resources, only CRUD API with no validation was implemented in moto.

Also fixed the logic while validating the request, we would call get_model not expecting it to raise an Exception.
(Same issue as in #7846 for validators, will add a proper test in the mentioned PR)

edit: modified a check in the validation for the model existence (previously in moto, now in our store) that wasn't caught with the tests, will add in a following PR.

@bentsku bentsku temporarily deployed to localstack-ext-tests March 9, 2023 17:18 — with GitHub Actions Inactive
@bentsku bentsku changed the base branch from master to apigw-parity-work-res-methods March 9, 2023 17:19
@bentsku bentsku force-pushed the apigw-parity-work-models branch from 6e63863 to 725c50d Compare March 9, 2023 17:22
@bentsku bentsku force-pushed the apigw-parity-work-res-methods branch from 456136b to d0f78f4 Compare March 10, 2023 09:09
@bentsku bentsku force-pushed the apigw-parity-work-models branch from dc8e27a to c0d94f6 Compare March 10, 2023 09:09
@coveralls
Copy link

coveralls commented Mar 10, 2023 •
edited
Loading

Coverage Status

Coverage: 85.111% (+0.01%) from 85.098% when pulling e1f6be5 on apigw-parity-work-models into d010e52 on master.

Base automatically changed from apigw-parity-work-res-methods to master March 13, 2023 12:04
@bentsku bentsku force-pushed the apigw-parity-work-models branch from 35cb8e3 to ba1e95c Compare March 13, 2023 12:48
@bentsku bentsku temporarily deployed to localstack-ext-tests March 13, 2023 12:48 — with GitHub Actions Inactive
@bentsku bentsku changed the title wip: apigw parity work Model resource apigw parity work on Model resource Mar 13, 2023
@bentsku bentsku temporarily deployed to localstack-ext-tests March 13, 2023 12:54 — with GitHub Actions Inactive
@bentsku bentsku marked this pull request as ready for review March 13, 2023 12:55
@bentsku bentsku temporarily deployed to localstack-ext-tests March 13, 2023 13:13 — with GitHub Actions Inactive
@github-actions
Copy link

github-actions bot commented Mar 13, 2023 •
edited
Loading

LocalStack integration with Pro

       3 files  ±  0         3 suites  ±0   1h 40m 32s ⏱️ +24s
1 796 tests  -   5  1 415 ✔️  -   5  381 💤 ±0  0 ❌ ±0 
2 514 runs   - 13  1 781 ✔️  - 13  733 💤 ±0  0 ❌ ±0 

Results for commit e1f6be5. ± Comparison against base commit d010e52.

♻️ This comment has been updated with latest results.

@bentsku bentsku mentioned this pull request Mar 13, 2023
Merged
@bentsku bentsku temporarily deployed to localstack-ext-tests March 14, 2023 01:49 — with GitHub Actions Inactive
@bentsku bentsku mentioned this pull request Mar 15, 2023
Merged
calvernaz
calvernaz approved these changes Mar 15, 2023
View reviewed changes
Copy link
Contributor

@calvernaz calvernaz left a comment

Choose a reason for hiding this comment

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

LGTM, nice set of changes. It scares be a bit that amount of work to pull the model into Localstack, given the amount of work just for Model is this much 😅

@bentsku
Copy link
Contributor Author

bentsku commented Mar 16, 2023

LGTM, nice set of changes. It scares be a bit that amount of work to pull the model into LocalStack, given the amount of work just for Model is this much 😅

Thanks! I guess pulling it into LocalStack looks like a lot of work because there was zero validation done on Moto side (so no work at all 😄), and now we have quite a good amount I'd say. Some resources will need even more work, this one wasn't so bad really, but then we can reuse some mechanisms we've encountered already.
And there's even more coming, with #7872 (this will be used by RequestValidator as well).

@bentsku bentsku force-pushed the apigw-parity-work-models branch from ea9006d to e1f6be5 Compare March 16, 2023 11:11
@bentsku bentsku temporarily deployed to localstack-ext-tests March 16, 2023 11:11 — with GitHub Actions Inactive
@bentsku bentsku merged commit 77aab87 into master Mar 16, 2023
@bentsku bentsku deleted the apigw-parity-work-models branch March 16, 2023 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dominikschubert dominikschubert Awaiting requested review from dominikschubert

1 more reviewer

@calvernaz calvernaz calvernaz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bentsku @coveralls @calvernaz