Skip to content

Conversation

@lizard-boy
Copy link

Motivation

Changes

maxhoheiser and others added 25 commits January 31, 2024 11:27
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This pull request introduces new tests and enhancements for Kinesis Firehose functionality in LocalStack, focusing on resharding operations and multiple delivery streams.

  • Added tests for Firehose with Kinesis as source, validating behavior during resharding (split and merge) operations
  • Implemented new test for multiple Firehose delivery streams connected to a single Kinesis stream
  • Enhanced Firehose provider to support unique DynamoDB lease tables for each delivery stream
  • Improved Kinesis mock server log level handling for better consistency with LocalStack logging
  • Updated fixtures in conftest.py to support more flexible Firehose and Kinesis testing scenarios

30 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings

if not is_api_enabled("cloudwatch"):
return
cw_client = connect_to(region_name=region_name).cloudwatch
cw_client = connect_to(aws_access_key_id=account_id, region_name=region_name).cloudwatch
Copy link

Choose a reason for hiding this comment

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

style: Consider using a consistent parameter order across functions (e.g., account_id before region_name)

const AWS = require("aws-sdk");
let dynamoDb;

var config = {};
Copy link

Choose a reason for hiding this comment

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

style: Consider using 'const' instead of 'var' for better scoping

let dynamoDb;

var config = {};
if (process.env.AWS_ENDPOINT_URL) {
Copy link

Choose a reason for hiding this comment

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

style: Consider using const instead of var for config object

Comment on lines +107 to +108
shard_id = shard["ShardId"][-2:]
starting_hash_keys[shard_id] = shard["HashKeyRange"]["StartingHashKey"]
Copy link

Choose a reason for hiding this comment

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

logic: Extracting only the last 2 characters of ShardId may not be reliable for all shard naming conventions

Comment on lines +128 to +129
json_array_string = "[" + input_string.replace("}{", "},{") + "]"
message = json.loads(json_array_string)
Copy link

Choose a reason for hiding this comment

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

style: This JSON parsing method assumes a specific format. Consider using a more robust parsing approach

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.

10 participants