feat: keep.join function for workflows #2547
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
dosubot
bot
added
the
size:S
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce new functionality across multiple modules. A new Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
keep/functions/__init__.py (2)
178-187: Add docstring to document function behavior and parameters.The new
joinfunction lacks documentation. Add a docstring to explain its purpose, parameters, return value, and examples.def join(iterable: list | dict | str, delimiter: str = ",") -> str: + """Join elements of an iterable into a delimited string. + + Args: + iterable: Input collection to join. Can be: + - list: Elements are converted to strings + - dict: Formatted as "key=value" pairs + - str: Parsed as JSON first + delimiter: String to join elements with (default: ",") + + Returns: + str: Joined string representation + + Examples: + >>> join(['a', 'b', 'c']) + 'a,b,c' + >>> join({'key': 'value'}) + 'key=value' + """
185-186: Consider enhancing type handling for nested structures.The current implementation might not handle nested dictionaries or lists properly. Consider adding recursive handling for nested structures.
if isinstance(iterable, dict): - return delimiter.join([f"{k}={v}" for k, v in iterable.items()]) + def format_value(v): + if isinstance(v, (dict, list)): + return json.dumps(v) + return str(v) + return delimiter.join([f"{k}={format_value(v)}" for k, v in iterable.items()]) return delimiter.join([str(item) for item in iterable])keep/iohandler/iohandler.py (2)
523-523: Add descriptive comments for the test data structureWhile the new tags-based structure is good, consider adding comments explaining the expected format and usage of the tags in the event context, especially since this is test code.
525-525: Enhance test case with assertions and documentationThe test case could be improved in several ways:
- Add expected output documentation
- Include assertions to validate the result
- Consider removing the unnecessary asteval evaluation for this simple string concatenation test
Example improvement:
# Test keep.join function for URL parameter construction expected = "https://www.keephq.dev?k1&k2" result = iohandler.render('https://www.keephq.dev?keep.join("{{alert.tags}}", "&")') assert result == expected, f"Expected {expected}, but got {result}"keep/providers/cloudwatch_provider/cloudwatch_provider.py (4)
260-260: InitializequerytoNoneinstead ofFalsefor clarityIt is more idiomatic in Python to initialize variables to
Nonewhen they are expected to hold a value or object that is not yet assigned. This improves code readability and avoids potential confusion with boolean values.Apply this diff to update the initialization:
- query = False + query = None
282-282: Initializequery_idtoNoneinstead ofFalsefor claritySimilarly, initializing
query_idtoNonemakes it clear that it has not been assigned a meaningful value yet.Apply this diff to update the initialization:
- query_id = False + query_id = None
Line range hint
284-287: Fix incorrect assignment toscopesdictionaryCurrently, the code assigns to
scopesusing a tuple as a key:scopes["logs:GetQueryResults", "logs:DescribeQueries"] = "..."This creates a single key that is a tuple, which is likely unintended and may lead to logical errors when accessing the scopes. Instead, assign to each key separately to accurately reflect the validation results.
Apply this diff to fix the issue:
- scopes["logs:GetQueryResults", "logs:DescribeQueries"] = ( + scopes["logs:GetQueryResults"] = ( "Could not validate logs:GetQueryResults scope without logs:DescribeQueries, so assuming the scope is not granted." + ) + scopes["logs:DescribeQueries"] = ( + "Could not validate logs:GetQueryResults scope without logs:DescribeQueries, so assuming the scope is not granted." + )
Line range hint
260-273: Clarify scope validation logic in exception handlingIn the
exceptblock while handling exceptions fromlogs_client.start_query, the logic assumes that aResourceNotFoundExceptionindicates that thelogs:StartQueryscope is not required:if "ResourceNotFoundException" in str(e): self.logger.info("AWS logs:StartQuery scope is not required") scopes["logs:StartQuery"] = TrueHowever, a
ResourceNotFoundExceptiontypically means that the log group does not exist, but the permissionlogs:StartQueryis granted. Therefore, the scopelogs:StartQueryshould be marked as granted (True) in this case. The log message should reflect that the scope is granted rather than not required.Consider updating the logic to correctly reflect the scope validation:
if "ResourceNotFoundException" in str(e): - self.logger.info("AWS logs:StartQuery scope is not required") + self.logger.info("AWS logs:StartQuery scope is granted") scopes["logs:StartQuery"] = True
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
keep/functions/__init__.py(1 hunks)keep/iohandler/iohandler.py(1 hunks)keep/providers/cloudwatch_provider/cloudwatch_provider.py(2 hunks)
dosubot
bot
added
size:M
Summary by CodeRabbit
New Features
joinfunction for concatenating various data types into a single string.IOHandlerclass using the newjoinfunction.CloudwatchProviderclass.Bug Fixes
Chores
pyproject.toml.