Introduce terraform subcommand #221
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe updates introduce significant enhancements to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Command
participant Terraform
User->>CLI: cpflow terraform generate
CLI->>Command: Initialize Generate Command
Command->>Terraform: Generate configuration files
Terraform-->>Command: Return generated files
Command-->>CLI: Display success message
CLI-->>User: Show confirmation of files generated
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
@coderabbitai review |
Actions performedReview triggered.
|
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- docs/commands.md (1 hunks)
- lib/command/base.rb (1 hunks)
- lib/command/base_sub_command.rb (1 hunks)
- lib/command/terraform/generate.rb (1 hunks)
- lib/cpflow.rb (4 hunks)
- spec/cpflow_spec.rb (1 hunks)
- spec/support/command_helpers.rb (3 hunks)
Additional context used
Markdownlint
docs/commands.md
447-447: null
Multiple headings with the same content(MD024, no-duplicate-heading)
Additional comments not posted (12)
lib/command/base_sub_command.rb (2)
9-14: LGTM!The
self.subcommand_prefixmethod correctly transforms the class name to a subcommand prefix.
5-7: Verify the necessity ofrubocop:disable.The
rubocop:disable Style/OptionalBooleanParametercomment might be necessary, but it's good to verify if there's a better way to handle this.spec/cpflow_spec.rb (1)
23-36: LGTM!The test case correctly verifies the handling of subcommands, ensuring that the
helpcommand and each subcommand return the expected results.spec/support/command_helpers.rb (3)
188-189: Ensure the global state is restored correctly.The ensure block correctly restores the original value of
$PROGRAM_NAME.
266-268: LGTM!The new
package_namemethod improves code readability and encapsulation.
160-162: Ensure the global state is preserved correctly.The changes to capture and set
$PROGRAM_NAMEare correct. Ensure that this does not affect other parts of the system.Verification successful
The global state of
$PROGRAM_NAMEis preserved correctly.The changes to capture and set
$PROGRAM_NAMEare confined to therun_cpflow_commandmethod, and the original value is restored properly. No unintended side effects were found in other parts of the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the global state of $PROGRAM_NAME is preserved correctly. # Test: Search for the usage of $PROGRAM_NAME. Expect: No unintended side effects. rg --type ruby -A 5 '\$PROGRAM_NAME'Length of output: 752
lib/cpflow.rb (2)
116-119: LGTM!The changes ensure that the help functionality correctly handles subcommands.
168-176: LGTM!The
klass_formethod enhances the extensibility of the command structure by allowing subcommands to be defined dynamically.lib/command/base.rb (1)
47-61: LGTM!The changes to the
all_commandsmethod improve the command discovery mechanism by supporting a hierarchical module structure and enabling the inclusion of commands from nested directories.docs/commands.md (3)
447-447: LGTM!The new
generatecommand heading is correctly placed.Tools
Markdownlint
447-447: null
Multiple headings with the same content(MD024, no-duplicate-heading)
449-450: LGTM!The description for the
generatecommand is clear and concise.
451-453: LGTM!The usage example for the
generatecommand is correctly formatted and clear.
I missed that 🥲 I still prefer subcommands because user can get all Another issue with this approach is class names which must include subcommand namespace in class names ( I won't push on subcommand approach @rafaelgomesxyz @borela WDYT? |
|
@zzaakiirr I am in favor of your design as it is more common in UNIX for CLIs that support subcommands. @justin808 @rafaelgomesxyz @dzirtusss @zzaakiirr I would prefer to change the loading logic to just reopen the CLI class for each command and declare them like Thor docs says, e.g.: By reopening the class this way, you can still share state, methods etc... and still follow Thor's design. |
|
@borela We have a bunch of custom logic that benefits from the current loading logic, as you can see in https://github.com/shakacode/control-plane-flow/blob/main/lib/cpflow.rb#L168. What are the benefits of switching to your proposed logic? Thor doesn't really have a design for defining commands in separate files, so I'm not sure what you mean by that. I think we should keep it the way it is. |
|
@rafaelgomesxyz, it looks like you are re-implementing Thor's features; Which features that custom logic implement that Thor already doesn't support? Validations? That could be extracted to a method that accepts a value and pattern to validate against. |
There was a problem hiding this comment.
@zzaakiirr Let's keep the subdomain approach then. I've finished reviewing the entire PR and added some more comments.
lib/command/base.rb
Outdated
| full_classname = [*namespaces, classname].join("::").prepend("::") | ||
|
|
||
| command_key = File.basename(file, ".rb") | ||
| prefix = namespaces[1..1].map(&:downcase).join("_") |
There was a problem hiding this comment.
Did you mean namespaces[1..]? Because namespaces[1..1] should be the same as namespaces[1], no?
There was a problem hiding this comment.
You are right, I meant namespaces[1..]. We don't need first namespace (which is Command)
There was a problem hiding this comment.
It's still namespaces[1..1] by the way.
|
@borela We're not though, we just have a layer on top of Thor to make things easier. It's not just validations, but also handling of deprecated commands and special args / options cases, as well as setting some fallback values here and there. Regardless, if we were to reopen the class in every command file, we'd have to repeat all of this logic for each file. Sure, we could move some logic to common helpers, but we'd still have repetition, which is why this way seems better to me. I don't see any benefits in switching to the way you proposed. |
|
@rafaelgomesxyz The code for each command would be linear and less magical. A little bit of repetition is fine if the end result is simpler to read. In an example scenario, command x is having a validation issue. I will go to the command x file and see how validation is being performed (either directly or by calling an accessory function) inside the command's method. That does not happen with the current design, it is not immediately obvious where simple stuff like validations are happening or that those options are being validated. In contrast, other projects using thor just do simple ||= for fallback values and validate the option on the command itself, which again can be extracted to a method and called by the command. |
|
Let's see what Sergey and Justin think, but I still think we should keep it as is. I think once you understand the constants defined at the top of the command classes and how options are defined in Another thing I like about the current approach is that we have a somewhat centralized place for all Thor stuff, which allows us to have a better separation of concerns, and detaches our app from Thor. It would be a lot easier to adapt the code if we wanted to replace Thor with something else - I know Justin was considering moving to an We also have some configs set on the class level based on the commands, which benefits from the current approach: control-plane-flow/lib/cpflow.rb Lines 246 to 248 in 82ff517 These cannot be set individually, so we'd still need a way to go through all command classes looking for matches. |
zzaakiirr
force-pushed
the
inroduce-terraform-subcommand
branch
from
c4074e9
to
ab93ec4
Compare
|
@rafaelgomesxyz Looking at the source, just build image has ACCEPTS_EXTRA_OPTIONS set to true, and it looks like it only does this because it forwards them to docker. Why not declare the docker options supported? The error message will be better and when somebody does |
|
@rafaelgomesxyz if zakir pr is following the current pattern and working we can merge this pr and I can move my suggestions to an issue. |
There was a problem hiding this comment.
@zzaakiirr Just one thing that you said you changed, but probably forgot to. Feel free to merge after changing it.
lib/command/base.rb
Outdated
| full_classname = [*namespaces, classname].join("::").prepend("::") | ||
|
|
||
| command_key = File.basename(file, ".rb") | ||
| prefix = namespaces[1..1].map(&:downcase).join("_") |
There was a problem hiding this comment.
It's still namespaces[1..1] by the way.
zzaakiirr
force-pushed
the
inroduce-terraform-subcommand
branch
from
ab93ec4
to
7eaf5ca
Compare
What does this PR do?
This PR adds possibility to add subcommands to
cpflowcli tool and addscpflow terraform generatecommand which does nothing (for now)Note
CommandHelpers#run_cpflow_commandso thatcpflow helpshowscpflow <command_name>i/orspec <command_name>Command::Terraform::Generatewill be implemented in feature PRsScreenshots
Summary by CodeRabbit
New Features
generatecommand to automate Terraform configuration file creation in the CLI.Bug Fixes
Tests
Chores