Enhancement | Catch and handle HCL parsing errors #296
Conversation
WalkthroughThe pull request introduces a new utility function, Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Auth/Terraform Module
participant Util as parse_tf_file()
participant HCL as hcl2.load
Caller ->> Util: Call parse_tf_file(filePath)
Util ->> HCL: Attempt to parse file
HCL -->> Util: Return parsed content or error
alt Successful Parsing
Util -->> Caller: Return parsed data
else Malformed HCL
Util -->> Caller: Raise ExitError with a user-friendly message
end
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (2)
✨ Finishing Touches
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
leverage/_utils.py(2 hunks)leverage/modules/auth.py(3 hunks)leverage/modules/terraform.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
leverage/_utils.py
127-127: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
leverage/modules/auth.py
6-6: hcl2 imported but unused
Remove unused import: hcl2
(F401)
🪛 GitHub Actions: Tests | Lint w/ Black
leverage/_utils.py
[warning] 1-1: 1 file would be reformatted. Please run Black to format the code.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration_tests_cli_refarch (3.9.15, 1.3.5-0.2.0)
🔇 Additional comments (3)
leverage/_utils.py (1)
4-4: LGTM!The imports are correctly organized and necessary for the new functionality.
Also applies to: 8-9
leverage/modules/auth.py (1)
69-70: LGTM!The changes correctly use the new
parse_tf_filefunction to handle HCL parsing errors gracefully.Also applies to: 78-79
leverage/modules/terraform.py (1)
515-516: LGTM!The changes correctly use the new
parse_tf_filefunction to handle HCL parsing errors gracefully, with a fallback to an empty dict if the file doesn't exist.
| def parse_tf_file(file: Path): | ||
| """ | ||
| Open and parse an HCL file. | ||
| In case of a parsing error, raise a user-friendly error. | ||
| """ | ||
| with open(file) as f: | ||
| try: | ||
| parsed = hcl2.load(f) | ||
| except lark.exceptions.UnexpectedInput: | ||
| raise ExitError(1, f"There is a parsing error with the {f.name} file. Please review it.") | ||
| else: | ||
| return parsed |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in parse_tf_file.
The error handling can be enhanced to:
- Catch FileNotFoundError for missing files
- Include the original error message for better debugging
- Chain exceptions using
raise ... from err
Apply this diff to improve error handling:
def parse_tf_file(file: Path):
"""
Open and parse an HCL file.
In case of a parsing error, raise a user-friendly error.
"""
with open(file) as f:
try:
parsed = hcl2.load(f)
- except lark.exceptions.UnexpectedInput:
- raise ExitError(1, f"There is a parsing error with the {f.name} file. Please review it.")
+ except FileNotFoundError as err:
+ raise ExitError(1, f"The file {f.name} does not exist.") from err
+ except lark.exceptions.UnexpectedInput as err:
+ raise ExitError(1, f"There is a parsing error with the {f.name} file. Please review it.\nError: {err}") from err
else:
return parsedCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.8.2)
127-127: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
leverage/modules/auth.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration_tests_cli_refarch (3.9.15, 1.3.5-0.2.0)
🔇 Additional comments (1)
leverage/modules/auth.py (1)
7-7: LGTM! Import changes are well-structured.The imports are properly organized and align with the new functionality.
Also applies to: 10-10
| backend_config = parse_tf_file(cli.paths.local_backend_tfvars) | ||
| tf_profile = backend_config["profile"] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for backend configuration parsing.
While the layer configuration files have error handling (try-except), the backend configuration parsing lacks similar protection. A malformed backend.tfvars could lead to unclear errors.
Consider wrapping the backend configuration parsing in a try-except block:
- backend_config = parse_tf_file(cli.paths.local_backend_tfvars)
- tf_profile = backend_config["profile"]
+ try:
+ backend_config = parse_tf_file(cli.paths.local_backend_tfvars)
+ tf_profile = backend_config["profile"]
+ except FileNotFoundError:
+ raise ExitError(40, f"Backend configuration file not found: {cli.paths.local_backend_tfvars}")
+ except KeyError:
+ raise ExitError(40, "Missing 'profile' in backend configuration")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| backend_config = parse_tf_file(cli.paths.local_backend_tfvars) | |
| tf_profile = backend_config["profile"] | |
| try: | |
| backend_config = parse_tf_file(cli.paths.local_backend_tfvars) | |
| tf_profile = backend_config["profile"] | |
| except FileNotFoundError: | |
| raise ExitError(40, f"Backend configuration file not found: {cli.paths.local_backend_tfvars}") | |
| except KeyError: | |
| raise ExitError(40, "Missing 'profile' in backend configuration") |
…/leverage into BL-294-friendly-parsing-errors
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
leverage/modules/credentials.py (1)
453-470: Strengthen credential validation security.The function assumes credentials are invalid only if the error code is 255 and contains "InvalidClientTokenId". This might miss other security-related errors.
- return error_code != 255 and "InvalidClientTokenId" not in output + if error_code != 0: + logger.debug(f"Credential validation failed with error: {output}") + return False + return True
♻️ Duplicate comments (1)
leverage/_utils.py (1)
119-130:⚠️ Potential issueImprove error handling in parse_tf_file.
The error handling can be enhanced to:
- Catch FileNotFoundError for missing files
- Include the original error message for better debugging
- Chain exceptions using
raise ... from errApply this diff to improve error handling:
def parse_tf_file(file: Path): """ Open and parse an HCL file. In case of a parsing error, raise a user-friendly error. """ with open(file) as f: try: parsed = hcl2.load(f) - except lark.exceptions.UnexpectedInput: - raise ExitError(1, f"There is a parsing error with the {f.name} file. Please review it.") + except FileNotFoundError as err: + raise ExitError(1, f"The file {f.name} does not exist.") from err + except lark.exceptions.UnexpectedInput as err: + raise ExitError(1, f"There is a parsing error with the {f.name} file. Please review it.\nError: {err}") from err else: return parsed🧰 Tools
🪛 Ruff (0.8.2)
128-128: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
🧹 Nitpick comments (5)
leverage/modules/run.py (1)
30-31: Add a docstring forTaskNotFoundError.Add a docstring to describe when this exception is raised, similar to the docstring in
MalformedTaskArgumentError.Apply this diff to add the docstring:
class TaskNotFoundError(RuntimeError): + """When a task specified by the user is not found in the build script""" passleverage/tasks.py (1)
26-27: Add a docstring forMissingParensInDecoratorError.Add a docstring to describe when this exception is raised.
Apply this diff to add the docstring:
class MissingParensInDecoratorError(ImportError): + """When the @task decorator is used without parentheses""" passleverage/modules/project.py (1)
281-298: Enhance the docstring forvalidate_config.The docstring should include details about the validation rules for project name and short name.
Apply this diff to improve the docstring:
def validate_config(config: dict): """ - Run a battery of validations over the config file (project.yaml). + Validate the configuration in project.yaml file. + + Args: + config (dict): Project configuration dictionary. + + Raises: + ExitError: If project_name is not a valid name (25 chars max, lowercase alphanumeric and hyphens only). + ExitError: If short_name is not valid (2-4 chars, lowercase alphabetic only). + + Returns: + bool: True if all validations pass. """leverage/modules/credentials.py (2)
148-182: Consider enhancing file permission checks.While the function validates file existence, it should also verify read permissions to prevent potential permission-related errors later.
- "validate": lambda value: (Path(value).expanduser().is_file() and Path(value).expanduser().exists()) - or "Path must be an existing file", + "validate": lambda value: ( + path := Path(value).expanduser(), + path.is_file() and path.exists() and os.access(path, os.R_OK) + )[1] or "Path must be an existing readable file",
702-714: Add type hints to improve code maintainability.Consider adding type hints to function parameters and return values for better code maintainability and IDE support.
-def configure( +def configure( type: str, - credentials_file, + credentials_file: Optional[Path], - fetch_mfa_device, + fetch_mfa_device: bool, - overwrite_existing_credentials, + overwrite_existing_credentials: bool, - skip_access_keys_setup, + skip_access_keys_setup: bool, - skip_assumable_roles_setup, + skip_assumable_roles_setup: bool, -): +) -> None:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
leverage/__init__.py(1 hunks)leverage/_internals.py(1 hunks)leverage/_utils.py(2 hunks)leverage/conf.py(1 hunks)leverage/leverage.py(1 hunks)leverage/logger.py(1 hunks)leverage/modules/credentials.py(1 hunks)leverage/modules/project.py(1 hunks)leverage/modules/run.py(1 hunks)leverage/path.py(1 hunks)leverage/tasks.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- leverage/init.py
- leverage/conf.py
🧰 Additional context used
🪛 Ruff (0.8.2)
leverage/_utils.py
128-128: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration_tests_cli_refarch (3.9.15, 1.3.5-0.2.0)
🔇 Additional comments (11)
leverage/leverage.py (1)
4-4: LGTM!leverage/_internals.py (1)
4-4: LGTM!leverage/logger.py (1)
4-4: LGTM!leverage/_utils.py (1)
5-5: LGTM!The imports are appropriate for the new functionality.
Also applies to: 9-10
leverage/modules/run.py (2)
66-68: LGTM!The error handling is consistent with the codebase's error handling patterns, and the error message is properly logged before exiting.
🧰 Tools
🪛 Ruff (0.8.2)
68-68: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
83-122: LGTM!The function's error handling has been improved with descriptive error messages and proper documentation of the exceptions in the docstring.
🧰 Tools
🪛 Ruff (0.8.2)
113-113: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
leverage/tasks.py (1)
30-71: LGTM!The function's error handling has been improved with descriptive error messages and proper documentation of the exceptions in the docstring.
leverage/path.py (2)
16-18: LGTM!The exception class is well-defined with a descriptive docstring.
38-58: LGTM!The function's error handling has been improved with proper exception handling and documentation.
🧰 Tools
🪛 Ruff (0.8.2)
54-54: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
55-55: Local variable
excis assigned to but never usedRemove assignment to unused variable
exc(F841)
56-56: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
leverage/modules/project.py (1)
259-279: LGTM!The function's error handling has been improved with proper exception handling and documentation.
🧰 Tools
🪛 Ruff (0.8.2)
278-278: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
leverage/modules/credentials.py (1)
1-68: LGTM! Well-organized imports and constants.The imports are properly organized, and the regex patterns are well-documented with references to AWS documentation.
Pull Request Test Coverage Report for Build 13227561880Details
💛 - Coveralls |
exequielrafaela
changed the title
What?
Catch the potential errors we could have while parsing HCL files, and return a user-friendly error.
Why?
To enhance the user experience.
References
closes #294Summary by CodeRabbit
New Features
Refactor