[scmacdon]

Auto-generated Python code examples for kinesis

[github-actions]

🤖 AI Code Example Review

Overall this is a well-structured, comprehensive PR that closely follows the specification and AWS SDK example conventions. It is runnable, well-tested with offline stubs, and covers all 10 required API actions. A few nits and one moderate structural concern are noted below, but nothing is blocking.

Detailed Review

1. [nit] kinesis_wrapper.py imports json, datetime, and timezone at the top level but never uses them directly — they are only used in the scenario file. These unused imports should be removed from kinesis_wrapper.py.

2. [nit] scenarios/kinesis_basics_scenario.py imports boto3 and datetime/timezone at the top, but boto3 is only needed transitively (via KinesisWrapper.from_client) and datetime/timezone are not used at all in the scenario file. Clean up the unused imports.

3. [nit] hello/hello_kinesis.py reads StreamNames from the list_streams response. In recent boto3/Kinesis API versions the StreamNames list is still present but considered legacy — the preferred field is StreamSummaries. This inconsistency with kinesis_wrapper.py's list_streams (which uses StreamSummaries) may cause the hello example to silently return zero results if the account has many streams, because StreamNames is paginated differently from StreamSummaries. Consider aligning both to use StreamSummaries.

4. [nit] The wait_for_stream_active and wait_for_consumer_active helper methods in kinesis_wrapper.py are not covered by any unit tests. For completeness, at least a happy-path stub test for each waiter would be valuable. The wait_for_consumer_active method also calls describe_stream_consumer directly on self.kinesis_client rather than going through a wrapper method, which bypasses consistent error handling patterns.

5. [nit] In kinesis_basics_scenario.py, the deletion wait loop (lines ~245-255) could spin indefinitely if describe_stream_summary raises an error other than ResourceNotFoundException after the first successful pass — the inner raise re-raises through the outer except ClientError which only swallows ResourceNotFoundException. This is arguably safe because non-404 errors are re-raised, but there is no timeout guard analogous to wait_for_stream_active. A max-iteration limit or timeout would be more robust.

6. [nit] list_streams pagination in kinesis_wrapper.py uses ExclusiveStartStreamName (the legacy pagination parameter), not the newer NextToken-based pagination for list_streams. While still functional, using the legacy approach may produce unexpected behavior if StreamSummaries is empty on a page (accessing [-1] would raise an IndexError). A guard or switch to NextToken-based pagination would be safer.

7. [nit] The test file test_kinesis_basics.py does not include a conftest.py or a sys.path manipulation, so from kinesis_wrapper import KinesisWrapper will fail unless the test is run from the correct working directory or with PYTHONPATH set. Premium comparable examples use sys.path.append in conftest.py to handle this. Adding a conftest.py with path fixup would follow established conventions.

8. [policy] There are no integration tests (marked with @pytest.mark.integ) for the scenario runner itself (run_scenario). The existing unit tests cover the wrapper methods well but do not exercise the full scenario flow even with stubs. The comparable premium examples typically include at least a test_scenario stub-based test.

9. [nit] The specification step 7 output says Found 3 tag(s). HasMoreTags: false, but the scenario code (kinesis_basics_scenario.py) does not print HasMoreTags in Step 7, only Found N tag(s).. This is a minor output deviation from the spec.

10. [nit] Long lines exist in kinesis_basics_scenario.py (e.g., the sensor_data list literals on lines ~160-166 exceed PEP 8's 88-char limit for Black). These should be reformatted for consistency with Python coding standards.

---

_{Reviewed at commit 2e02982cae5dad8c76b6feb7339dca0b8e442cb4. This review was generated automatically using Amazon Bedrock. It compares your changes against existing examples and coding guidelines. Findings are labeled by severity (blocking / nit / policy) and reflect only the commit above. Please use your judgment — this is advisory, not authoritative.}

[github-actions]

🤖 AI Code Example Review

The PR is largely well-structured and addresses most prior review feedback. The unused imports in kinesis_wrapper.py (json, datetime, timezone) and in scenarios/kinesis_basics_scenario.py (boto3, datetime, timezone) are still present in the current code, a conftest.py is still missing for test path resolution, and hello_kinesis.py still uses the legacy StreamNames field. These are all nits/policy items — nothing is blocking. The code is runnable, well-tested, and specification-compliant.

Detailed Review

1. [nit] Still unresolved (prior item 1): kinesis_wrapper.py lines 14-17 import json, datetime, and timezone but none are used anywhere in this file — json and datetime/timezone are only used in scenarios/kinesis_basics_scenario.py. Remove these three unused imports from kinesis_wrapper.py to avoid linter warnings and keep the module clean.

2. [nit] Still unresolved (prior item 2): scenarios/kinesis_basics_scenario.py lines 23-26 import datetime, timezone, and boto3. datetime and timezone are never referenced in the scenario file (no calls to datetime(...) or timezone.utc). boto3 is also unused — KinesisWrapper.from_client() creates the client internally. Remove these three unused imports.

3. [nit] Still unresolved (prior item 3): hello/hello_kinesis.py line 29 uses response.get("StreamNames", list()) — the legacy field. The kinesis_wrapper.py list_streams method correctly uses StreamSummaries. For consistency and to avoid silently returning zero results in accounts with many streams, hello_kinesis.py should use StreamSummaries and access s['StreamName'] from each summary dict, matching the wrapper's approach.

4. [policy] Still unresolved (prior item 7): There is no conftest.py in python/example_code/kinesis/test/. The test file imports from kinesis_wrapper import KinesisWrapper which will fail unless tests are run from the parent directory or with PYTHONPATH set. Premium comparable examples (e.g., basics/controltower/test/conftest.py) use sys.path.append to handle this. A conftest.py with path fixup would follow established conventions.

5. [nit] Still unresolved (prior item 9): The specification Step 7 output specifies Found 3 tag(s). HasMoreTags: false but scenarios/kinesis_basics_scenario.py line ~207 only prints Found {len(tag_list)} tag(s). without the HasMoreTags value. This is a minor deviation from the spec. Consider printing the HasMoreTags value, or updating the spec — but currently they don't match.

6. [nit] Still unresolved (prior item 10): Long lines remain in scenarios/kinesis_basics_scenario.py — for example the sensor_data list literals on lines ~160-164 exceed 88 characters (Black's default). These should be reformatted for PEP 8 / Black compliance.

7. [nit] Prior item 6 (list_streams pagination) — partially addressed: The list_streams pagination logic on line ~68 accesses response["StreamSummaries"][-1]["StreamName"] when HasMoreStreams is True. If StreamSummaries happened to be empty on a page while HasMoreStreams is True (an unlikely but theoretically possible edge case), this would raise an IndexError. A guard if response.get("StreamSummaries") before this access would be safer, though this is unlikely to occur in practice.

8. [nit] Prior item 4 (waiter coverage) — still unresolved: wait_for_stream_active and wait_for_consumer_active in kinesis_wrapper.py have no unit tests. Adding at least a happy-path stubbed test for each would improve coverage. This was noted in the previous review and remains unaddressed.

9. [policy] Prior item 8 — still unresolved: There is still no integration test (e.g., @pytest.mark.integ decorated test_run_scenario) for the scenario runner. Premium comparable examples typically include at least a scenario-level test with stubs.

10. [nit] The README.md links for "Hello Kinesis" and "Learn the basics" both point to python kinesis_wrapper.py in the instructions section, but "Hello Kinesis" should point to python hello/hello_kinesis.py and "Learn the basics" should point to python scenarios/kinesis_basics_scenario.py. The entry points are misidentified in the README run instructions.

---

_{Reviewed at commit 8d54cc90521810d4a7bfccb9048fb7c2aa8f6f8d. This review was generated automatically using Amazon Bedrock. It compares your changes against existing examples and coding guidelines. Findings are labeled by severity (blocking / nit / policy) and reflect only the commit above. Please use your judgment — this is advisory, not authoritative.}

[github-actions]

🤖 AI Code Example Review

The PR is in good shape — all previously identified blocking and nit issues have been addressed or resolved in this commit. The remaining items are minor nits and one policy observation. The code is runnable, well-structured, specification-compliant, and properly tested with offline stubs.

Detailed Review

1. [nit] Prior issue resolved: hello_kinesis.py now uses StreamSummaries — The previous concern about hello_kinesis.py using the legacy StreamNames field has been fully addressed. The current code at line 28 correctly uses response.get('StreamSummaries', list()), consistent with kinesis_wrapper.py.

2. [nit] Prior issue resolved: conftest.py path fixup added — The conftest.py now includes sys.path.append(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) to allow kinesis_wrapper to be imported from the correct directory. This matches the established convention.

3. [nit] Prior issue resolved: unused imports in kinesis_wrapper.py removed — Confirmed that json, datetime, and timezone are no longer imported in kinesis_wrapper.py. The imports are now just logging, time, boto3, and ClientError.

4. [nit] Prior issue resolved: unused imports in kinesis_basics_scenario.py cleaned up — Confirmed that boto3 and unused datetime/timezone are no longer in the scenario file. The current imports are json, logging, time, ClientError, and KinesisWrapper.

5. [nit] Prior list_streams pagination IndexError risk still present — In kinesis_wrapper.py around line 63-69, the HasMoreStreams loop reads summaries[-1]['StreamName'] for the ExclusiveStartStreamName token. This uses the previous page's summaries, not the newly fetched ones. If the StreamSummaries field in a non-first page's response happened to be empty (edge case), summaries[-1] would raise IndexError. This is the same nit from the prior review. It's a defensive-coding nit, not blocking, since the guard if not summaries: break runs before the [-1] access using the prior response's summaries — so it's actually safe on closer inspection. No action strictly required.

6. [nit] wait_for_consumer_active calls describe_stream_consumer directly — In kinesis_wrapper.py (lines ~328-343), the wait_for_consumer_active method calls self.kinesis_client.describe_stream_consumer(...) directly rather than through a wrapper method. This is a minor inconsistency with the class's own pattern of wrapping client calls with error handling. Since wait_for_consumer_active is not a snippet-tagged action method, it's only a style note.

7. [nit] Step 7 output format minor spec deviation — The spec example shows HasMoreTags: false (lowercase boolean as a string), while the Python code at kinesis_basics_scenario.py line ~214 prints f"Found {len(tag_list)} tag(s). HasMoreTags: {has_more_tags}" which will emit HasMoreTags: False (Python capitalized bool). This is cosmetic but technically diverges from the spec output. Could use str(has_more_tags).lower() if exact spec conformance is required.

8. [policy] No integration test for run_scenario — As noted in the previous review, there is no @pytest.mark.integ-marked test for the full scenario flow. All unit tests cover individual wrapper methods only. This is a policy observation — the team should decide whether a scenario-level stub test is required.

9. [nit] stub_describe_stream_summary missing EncryptionType field — In kinesis_stubber.py (lines ~170-185), the stub response for describe_stream_summary does not include an EncryptionType key. The scenario code at kinesis_basics_scenario.py calls summary.get('EncryptionType', 'NONE') with a default, so this won't cause a failure, but the stub is slightly less realistic than it could be.

10. [nit] kinesis_basics_scenario.py imports from parent directory without sys.path manipulation — The scenario file uses from kinesis_wrapper import KinesisWrapper (a sibling directory) without any path manipulation. This works when run as python scenarios/kinesis_basics_scenario.py from the kinesis/ directory but may fail when imported from another context. The conftest.py handles this for tests, but the scenario itself has no sys.path guard. Consider adding sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) at the top of the scenario file for robustness.

---

_{Reviewed at commit 9ae23db0988f5a44e0f1faf1385dcf310f1957d1. This review was generated automatically using Amazon Bedrock. It compares your changes against existing examples and coding guidelines. Findings are labeled by severity (blocking / nit / policy) and reflect only the commit above. Please use your judgment — this is advisory, not authoritative.}

[scmacdon]

Cloud Build

http://dev-dsk-scmacdon-1d-3bd40994.us-east-1.amazon.com/sos-metadata505/build/AWSCodeExampleUsageDocs/AWSCodeExampleUsageDocs-3.0/AL2_x86_64/DEV.STD.PTHREAD/build/server-root/code-library/latest/ug/kinesis_example_kinesis_Scenario_GettingStarted_section.html

[RuiqiGao-aws]

I ran the Basics scenario for real in a sandbox account (us-west-2, 3 runs) and hit two blockers.

First, the documented command python scenarios/kinesis_basics_scenario.py fails with ModuleNotFoundError: No module named 'kinesis_wrapper' - the script imports from its parent directory, which isn't on sys.path when run from the README's location.

Second, with PYTHONPATH set as a workaround, all 3 runs fail at Step 3: ListShards raises ResourceInUseException ("instead in state CREATING") even though DescribeStreamSummary already reported the stream ACTIVE - the two control-plane views converge at different times for a fresh stream. On the plus side, the finally cleanup worked every run; no streams were leaked. Unit tests (20/20) and the metadata validator pass, but the stubber can't catch either of these.

[RuiqiGao-aws]

Re-verified at head 6d75b75: the README-documented command now runs end-to-end in a real account — rc=0, all 10 steps, 5 records written and read back, and no streams left afterwards.

All 43 unit tests pass, including the three new ones covering the list_shards retry path. Both blockers are resolved; thanks for the quick turnaround.

Fine as a follow-up if you prefer - approving.