Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #47 +/- ##
==========================================
- Coverage 83.87% 81.22% -2.65%
==========================================
Files 9 10 +1
Lines 186 245 +59
Branches 14 17 +3
==========================================
+ Hits 156 199 +43
- Misses 26 41 +15
- Partials 4 5 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
calebsyring
left a comment
There was a problem hiding this comment.
Requested some changes.
A few of them will depend on a PR I should get out shortly.
src/critic/monitor_utility.py
Outdated
| with t.batch_writer() as bw: | ||
| for i in range(1, count + 1): | ||
| slug = f'{prefix}-{i:04d}' | ||
| bw.put_item(Item={'project_id': project_id, 'slug': slug}) |
There was a problem hiding this comment.
Part of the point of this is to have operational monitors. So that we can actually run checks at scale.
These at least need a url (you can use google.com or something if you want) and probably some other fields. Maybe just make sure that your fake data passes the model validation.
src/critic/app.py
Outdated
| @app.route('/monitors/create') | ||
| def create_monitors_route(): | ||
| # SImple start to get monitor going | ||
| table_name = os.environ.get('MONITOR_TABLE_NAME', 'Critic-Monitors') |
There was a problem hiding this comment.
You're right that table names probably need to be defined via environment variables.
I'll take care of that in a pr I'm working on.
src/critic/app.py
Outdated
| project_id=project_id, | ||
| prefix=prefix, | ||
| count=count, | ||
| ddb=None, # Uses a boto3 ddb resource |
There was a problem hiding this comment.
I'll also have a universal ddb instance that can be accessed instead of passing it through all the time.
src/critic/app.py
Outdated
| raise RuntimeError('Deliberate runtime error') | ||
|
|
||
|
|
||
| @app.route('/monitors/create') |
There was a problem hiding this comment.
These should be CLI commands and not routes. These are open to the public and anyone could hit them. It's also a somewhat clunky UX to have to go that url for your lambda instead of just calling it via CLI.
There's a example here in mu for exposing CLI commands: https://github.com/level12/mu/blob/master/examples/mu_hello/mu_hello.py
src/critic_tests/monitor_test.py
Outdated
| TABLE_NAME = 'UptimeMonitor' | ||
|
|
||
|
|
||
| def _create_test_table(): |
There was a problem hiding this comment.
Shouldn't need this because of
critic/tests/critic_tests/conftest.py
Line 41 in 6e58869
Wait to fix until I get my PR out and you rebase. It'll be easier to use.
| # Create 10 monitors | ||
| created = create_monitors( | ||
| table_name=TABLE_NAME, | ||
| project_id=project_id, |
There was a problem hiding this comment.
Note that this assumes the existence of a project.
We'll need some way to create a project. Maybe you're just planning on that being done in the console manually?
7c53137 to
1461832
Compare
src/critic/cli.py
Outdated
| @@ -1,2 +1,10 @@ | |||
| from critic.app import cli | |||
There was a problem hiding this comment.
The point of this file is to have cli stuff in here. See https://github.com/level12/coppy/blob/main/src/coppy/cli.py
We should be able to just import it for the ActionHandler as needed.
src/critic/monitor_utility.py
Outdated
| with t.batch_writer() as bw: | ||
| for i in range(1, count + 1): | ||
| slug = f'{prefix}-{i:04d}' | ||
| bw.put_item(Item=_monitor_item(project_id=project_id, slug=slug)) |
There was a problem hiding this comment.
Use the put method on the table. Should vastly simplify this file. The batch writer isn't that important here AFAIK
src/critic_tests/monitor_test.py
Outdated
| @@ -0,0 +1,80 @@ | |||
| import boto3 | |||
| from boto3.dynamodb.conditions import Key | |||
| from moto import mock_aws | |||
There was a problem hiding this comment.
Shouldn't need this. AWS should be mocked automatically for any test not marked an integration test.
src/critic_tests/monitor_test.py
Outdated
| from critic.tables import UptimeMonitorTable | ||
|
|
||
|
|
||
| def _create_test_table(ddb): |
There was a problem hiding this comment.
Shouldn't need to create tables... If you have to, something is wrong with moto_for_unit_tests
…s. No more manual monitor creations within the tests. Added new fixtures and conftest files to assist with testing, may not be needed but helped validate functionality.
|
Closing in favor of #59 |
Addresses issue #29.
Tests are working locally and inside a Docker container using pytest with a mocked DynamoDB environment. Monitor creation and deletion are functioning as expected. Next steps will need to focus on integration testing against real AWS resources and ensuring attributes match the assigned architecture.