Add skip_output_write option to register_module and update output.facets.yaml handling #66
Conversation
…ets.yaml handling - Introduced skip_output_write parameter in register_module to control output file generation. - Enhanced preview_module to conditionally generate output.facets.yaml based on the new option.
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Parser
participant FileSystem
participant RegisterModule
User->>CLI: preview_module [--skip-output-write]
CLI->>Parser: parse outputs.tf
Parser-->>CLI: output_interfaces, output_attributes
alt outputs exist
CLI->>FileSystem: write output-lookup-tree.json
alt output.facets.yaml does not exist and not --skip-output-write
CLI->>FileSystem: write output.facets.yaml
else output.facets.yaml exists or --skip-output-write
CLI-->>FileSystem: skip YAML generation
end
CLI->>RegisterModule: register_module(skip_output_write)
else no outputs
CLI-->>User: print "No outputs found"
end
CLI->>FileSystem: cleanup temp files
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
🧹 Nitpick comments (7)
ftf_cli/operations.py (1)
9-9: Remove unused import.The
ioimport is not used anywhere in the file.-import ioftf_cli/commands/preview_module.py (2)
16-16: Remove unused import.The
hclimport is not used in this file.-import hcl
223-223: Remove unnecessary f-string prefix.The string doesn't contain any placeholders, so the
fprefix is unnecessary.- click.echo(f"output.facets.yaml already exists, skipping generation.") + click.echo("output.facets.yaml already exists, skipping generation.")tests/commands/test_preview_module.py (4)
574-574: Remove unused import.The
builtinsimport is not used in the test.- import builtins
556-600: Comprehensive test with room for improvement.The test effectively validates the output.facets.yaml structure and cleanup, but has some issues:
- The test setup is quite complex with manual file manipulation
- The unused
resultvariable should be removed or used for assertions- The file creation at the end for structure validation seems out of place
Consider simplifying the test by either:
- Using the actual generated file content before cleanup, or
- Splitting into separate tests for generation and cleanup
- import builtins import yaml as _yaml import os as _os removed_files = [] orig_remove = _os.remove def fake_remove(path): removed_files.append(path) if os.path.basename(path) != "output.facets.yaml": orig_remove(path) with patch("os.remove", fake_remove): - result = runner.invoke( + runner.invoke( preview_module, [temp_module_with_outputs, "--profile", "default"] ) output_facets_path = os.path.join(temp_module_with_outputs, "output.facets.yaml") # It should have been created and then deleted assert output_facets_path in removed_files - # Recreate to check structure - with open(output_facets_path, "w") as f: - _yaml.dump({"out": {"interfaces": {"foo": "bar"}, "attributes": {"baz": "qux"}}}, f) - with open(output_facets_path, "r") as f: - data = _yaml.safe_load(f) - assert "out" in data - assert "interfaces" in data["out"] - assert "attributes" in data["out"] - assert "properties" not in data - assert "out" not in data["out"]
601-627: Good test coverage with minor cleanup needed.The test properly validates the skip functionality and parameter passing, but the
resultvariable is unused.- result = runner.invoke( + runner.invoke( preview_module, [temp_module_with_outputs, "--profile", "default", "--skip-output-write", "true"] )
628-656: Effective no-overwrite test with minor cleanup.The test correctly validates that existing files are not overwritten, but the
resultvariable is unused.- result = runner.invoke( + runner.invoke( preview_module, [temp_module_with_outputs, "--profile", "default"] )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ftf_cli/commands/preview_module.py(6 hunks)ftf_cli/operations.py(3 hunks)tests/commands/test_preview_module.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
ftf_cli/operations.py
9-9: io imported but unused
Remove unused import: io
(F401)
127-127: Use a context manager for opening files
(SIM115)
ftf_cli/commands/preview_module.py
16-16: hcl imported but unused
Remove unused import: hcl
(F401)
223-223: f-string without any placeholders
Remove extraneous f prefix
(F541)
tests/commands/test_preview_module.py
574-574: builtins imported but unused
Remove unused import: builtins
(F401)
584-584: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
618-618: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
649-649: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
🪛 Pylint (3.3.7)
ftf_cli/operations.py
[refactor] 127-127: Consider using 'with' for resource-allocating operations
(R1732)
tests/commands/test_preview_module.py
[refactor] 559-559: Too many arguments (7/5)
(R0913)
[refactor] 559-559: Too many positional arguments (7/5)
(R0917)
[refactor] 559-559: Too many local variables (17/15)
(R0914)
[refactor] 604-604: Too many arguments (7/5)
(R0913)
[refactor] 604-604: Too many positional arguments (7/5)
(R0917)
[refactor] 631-631: Too many arguments (7/5)
(R0913)
[refactor] 631-631: Too many positional arguments (7/5)
(R0917)
🔇 Additional comments (8)
ftf_cli/operations.py (3)
70-70: LGTM! Well-designed parameter addition.The new
skip_output_writeparameter follows good practices with a sensible default value and clear typing.
113-113: LGTM! Proper condition update.The condition correctly includes the new
skip_output_writeparameter to ensure metadata is prepared when needed.
121-121: LGTM! Consistent metadata field naming.The
skipOutputWritefield follows the camelCase convention used for other metadata fields.ftf_cli/commands/preview_module.py (5)
65-71: LGTM! Well-designed CLI option.The new
--skip-output-writeoption is properly configured with appropriate help text and validation.
85-124: Excellent refactoring! Improved modularity and maintainability.Breaking down the output generation into smaller, focused functions significantly improves code readability and maintainability. Each function has a single responsibility and clear purpose.
194-224: LGTM! Proper conditional generation logic.The implementation correctly handles:
- Conditional generation based on
skip_output_writeflag- Checking for existing files to avoid overwrites
- Proper error handling with user feedback
- Setting the output path for cleanup
235-235: LGTM! Proper parameter passing.The
skip_output_writeparameter is correctly passed to theregister_modulefunction.
262-268: LGTM! Consistent cleanup implementation.The cleanup logic for
output.facets.yamlfollows the same pattern as the existing cleanup foroutput-lookup-tree.json, ensuring consistency and proper resource management.
…losure and cleanup - Updated register_module to use a context manager for opening the metadata file, ensuring it is properly closed after use. - Simplified the logic for removing the temporary metadata file, ensuring it is always deleted after the request is made. - Removed unused import from preview_module.py to clean up the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ftf_cli/operations.py (1)
123-140: Good improvement: Addresses resource management concerns.The refactored metadata handling properly addresses the resource management issues flagged in past reviews. The implementation now uses context managers and ensures proper cleanup with try/finally blocks.
Key improvements:
- Temporary file creation and writing is properly handled
- File is opened with context manager for the request
- Cleanup is guaranteed via finally block
- Request is made within the file context
This is a significant improvement over the previous implementation.
🧹 Nitpick comments (2)
ftf_cli/operations.py (1)
9-9: Remove unused import.The
iomodule is imported but never used in the code.-import ioftf_cli/commands/preview_module.py (1)
222-222: Fix unnecessary f-string formatting.The f-string has no placeholders and can be simplified to a regular string.
- click.echo(f"output.facets.yaml already exists, skipping generation.") + click.echo("output.facets.yaml already exists, skipping generation.")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ftf_cli/commands/preview_module.py(5 hunks)ftf_cli/operations.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
ftf_cli/commands/preview_module.py
222-222: f-string without any placeholders
Remove extraneous f prefix
(F541)
ftf_cli/operations.py
9-9: io imported but unused
Remove unused import: io
(F401)
🔇 Additional comments (9)
ftf_cli/operations.py (3)
70-70: LGTM: Parameter addition is well-implemented.The new
skip_output_writeparameter follows good practices with a sensible default value and maintains backward compatibility.
113-113: LGTM: Condition logic correctly includes new parameter.The condition properly includes
skip_output_writeto ensure metadata is prepared when this flag is set, even if other git-related parameters are not provided.
121-121: LGTM: Metadata field addition is consistent.The
skipOutputWritefield is correctly added to the metadata dictionary using camelCase naming convention consistent with other fields.ftf_cli/commands/preview_module.py (6)
64-70: LGTM: CLI option is well-configured.The new
--skip-output-writeoption is properly defined with appropriate validation, help text, and default values.
80-80: LGTM: Function signature updated correctly.The parameter addition maintains consistency with the CLI option definition.
84-122: Excellent refactoring: Improved code organization.The monolithic output processing logic has been well-refactored into focused, single-responsibility functions:
parse_outputs_tf: Clean HCL2 parsing with proper file handlingextract_output_structures: Clear data extraction logicwrite_output_lookup_tree: Dedicated JSON file generationwrite_output_facets_yaml: Dedicated YAML file generationThis modularization significantly improves readability, testability, and maintainability.
194-223: LGTM: Output generation logic is well-implemented.The refactored logic correctly:
- Parses outputs.tf when present with proper error handling
- Conditionally generates both JSON and YAML files based on available data
- Respects the
skip_output_writeflag for YAML generation- Provides informative user messages for each operation
- Handles edge cases like missing files and existing output files
The implementation is robust and user-friendly.
234-234: LGTM: Parameter passing is consistent.The
skip_output_writeflag is correctly passed to theregister_modulefunction, maintaining consistency between the CLI and operations layer.
261-267: LGTM: Comprehensive cleanup logic.The cleanup logic has been properly extended to handle the new
output.facets.yamlfile, ensuring no temporary files are left behind. The error handling matches the existing pattern for the JSON file cleanup.
…preview_module.py for clarity
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
ftf_cli/commands/preview_module.py (4)
84-90: Consider adding explicit encoding for file operations.The file is opened without specifying encoding, which could lead to issues with non-ASCII characters in different environments.
- with open(output_file, "r") as file: + with open(output_file, "r", encoding="utf-8") as file:
107-108: Consider adding explicit encoding for file operations.Similar to the earlier file operation, specify encoding for consistency and reliability.
- with open(output_json_path, "w") as file: + with open(output_json_path, "w", encoding="utf-8") as file:
113-113: Move import to module level for better performance.Dynamic imports inside functions can impact performance and make dependencies less obvious.
+from ftf_cli.utils import generate_output_tree + def write_output_facets_yaml(path, output_interfaces, output_attributes): output_facets_file = os.path.join(path, "output.facets.yaml") - from ftf_cli.utils import generate_output_tree
120-121: Consider adding explicit encoding for file operations.Maintain consistency with encoding specification for YAML file writing.
- with open(output_facets_file, "w") as f: + with open(output_facets_file, "w", encoding="utf-8") as f:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ftf_cli/commands/preview_module.py(5 hunks)ftf_cli/operations.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ftf_cli/operations.py
🔇 Additional comments (5)
ftf_cli/commands/preview_module.py (5)
64-70: LGTM! Clean CLI option definition.The new
--skip-output-writeoption is well-defined with clear help text and proper boolean validation.
92-96: LGTM! Clean extraction logic.The function properly extracts the required data structures with appropriate default values.
194-223: LGTM! Well-structured conditional logic.The refactored logic properly handles the different scenarios:
- Parsing outputs.tf when it exists
- Generating output files conditionally based on skip_output_write flag
- Proper error handling and user feedback
- File existence checks before generation
The implementation correctly integrates the new functionality while maintaining backward compatibility.
234-234: LGTM! Proper parameter passing.The
skip_output_writeparameter is correctly passed to theregister_modulefunction.
261-267: LGTM! Comprehensive cleanup logic.The cleanup logic properly handles removal of the temporary
output.facets.yamlfile with appropriate error handling, mirroring the existing cleanup pattern for the JSON file.
…s and clean up imports - Added UTF-8 encoding to file read/write operations for better compatibility. - Removed redundant import statement for generate_output_tree to streamline the code.
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)
ftf_cli/commands/preview_module.py(9 hunks)
🔇 Additional comments (8)
ftf_cli/commands/preview_module.py (8)
3-6: New imports look appropriate for the functionality.The added imports support the new features:
getpassfor user identification,yamlfor YAML processing,hcl2for Terraform file parsing, andjsonfor JSON handling.
64-70: CLI option implementation is well-structured.The
--skip-output-writeoption includes proper validation and clear help text that explains its purpose.
84-90: Output parsing function is correctly implemented.The function properly checks for file existence and uses appropriate encoding. The error handling is implicit through the caller's try-catch blocks.
98-109: Output lookup tree function is well-implemented.The function correctly constructs the output structure and handles file writing with proper encoding. The error handling is managed by the caller.
111-121: YAML output function is properly structured.The function uses the appropriate utility function and creates the correct YAML structure with proper file handling.
193-221: Output generation logic is well-structured with good error handling.The conditional logic properly checks for required data before generating files, includes appropriate error handling, and provides clear user feedback. The file existence check prevents overwriting existing
output.facets.yamlfiles.
233-233: Module registration call correctly includes the new parameter.The
skip_output_writeparameter is properly passed to theregister_modulefunction, aligning with the PR objectives.
260-266: Cleanup logic properly handles temporary files.The cleanup code correctly removes the generated
output.facets.yamlfile with appropriate error handling, matching the existing pattern for the JSON file cleanup.
…d robustness - Enhanced the extract_output_structures function to handle various input types for locals, output_interfaces, and output_attributes, ensuring more reliable data extraction. - Added checks for list and dictionary types to prevent potential errors during extraction.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ftf_cli/commands/preview_module.py (1)
64-70: Clarify the help text for better user understanding.The help text mentions "Do not update the output type in facets" but the actual functionality is about controlling the generation of the
output.facets.yamlfile. Consider making this more explicit.- help="Do not update the output type in facets. Set to true only if you have already registered the output type before calling this command.", + help="Skip generating the output.facets.yaml file. Set to true only if the output schema has already been registered.",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ftf_cli/commands/preview_module.py(9 hunks)
🔇 Additional comments (10)
ftf_cli/commands/preview_module.py (10)
3-6: New imports look appropriate for the functionality.The added imports (getpass, yaml, hcl2, json, generate_output_tree) are all necessary for the new output processing features.
Also applies to: 12-12
84-90: Good defensive programming in parse_outputs_tf.The function properly handles the case where
outputs.tfdoesn't exist and uses UTF-8 encoding for file operations.
92-113: Excellent improvement addressing the previous IndexError concern.The function now properly handles different data structures (list/dict) with appropriate type checking and fallbacks. This addresses the past review comment about potential IndexError issues.
115-126: Well-structured output lookup tree generation.The function properly constructs the output structure and uses appropriate encoding for JSON serialization.
128-138: Clean YAML generation with proper structure.The function creates a well-structured YAML output using the utility function and maintains consistent encoding practices.
168-168: Good practice adding explicit UTF-8 encoding.Adding explicit UTF-8 encoding to all file operations ensures consistent behavior across different environments and prevents encoding-related issues.
Also applies to: 186-186, 190-190, 263-263
208-227: Robust error handling for output processing.The logic properly handles cases where
outputs.tfdoesn't exist or parsing fails, with appropriate user feedback and graceful degradation.
228-238: Well-implemented conditional file generation.The logic correctly respects the
--skip-output-writeflag and handles existing files appropriately. The error handling ensures the process continues even if YAML generation fails.
250-250: Proper parameter passing to register_module.The
skip_output_writeparameter is correctly passed to the registration function.
277-283: Thorough cleanup of temporary files.The cleanup logic properly handles both the JSON and YAML temporary files with appropriate error handling, ensuring no temporary files are left behind.
Summary by CodeRabbit
New Features
Bug Fixes
Tests