Skip to content

Conversation

@staticfox
Copy link

This implements the following changes and adjustments to the cmd module:

  • Reduced the amount of global variables to 1. Now the cmd module can either reference a global runtimeFlags variable which contains all the flags passed in from the command line as well as some new helper functions and a helper method for reducing the amount of copy/pasted logic.

  • Implemented a new GetConfig() function which caches the result of the first lookup as well as cleans up the amount of copy/pasted logic that was otherwise scattered throughout the module.

  • Moved each command's implementation in to named functions to improve future testability as well as allow us to tab over the entire function's logic by one.

  • Moved the definition of the commotion root directory from the template module in to the cmd module. This now means all directories are now pushed down to the templates module instead of being split between the templates module and the cmd module. Additionally, this will also make it easier to potentially allow re-defining the commotion root directory either via a command line argument or environment variable.

  • Added a new --repo_url flag to override the git URL that Terraform templates are cloned from. Like above, this means that the templates module now simply clones from the URL passed in instead of attempt to set a default on its own if an empty string or nil is passed in, which should never happen.

  • Various tweaks to logging. This adjusts certain log statements to run log.Printf instead of calling log.Println(... fmt.Sprintf). The rest of the code and locations these are used will be unified in a follow up patch.

This implements the following changes and adjustments to the cmd module:

* Reduced the amount of global variables to 1. Now the cmd module can either
  reference a global `runtimeFlags` variable which contains all the flags
  passed in from the command line as well as some new helper functions and
  a helper method for reducing the amount of copy/pasted logic.

* Implemented a new `GetConfig()` function which caches the result of the first
  lookup as well as cleans up the amount of copy/pasted logic that was otherwise
  scattered throughout the module.

* Moved each command's implementation in to named functions to improve future
  testability as well as allow us to tab over the entire function's logic by
  one.

* Moved the definition of the commotion root directory from the template module
  in to the cmd module. This now means all directories are now pushed down to the
  templates module instead of being split between the templates module and the
  cmd module. Additionally, this will also make it easier to potentially allow
  re-defining the commotion root directory either via a command line argument
  or environment variable.

* Added a new --repo_url flag to override the git URL that Terraform templates are
  cloned from. Like above, this means that the templates module now simply clones
  from the URL passed in instead of attempt to set a default on its own if an empty
  string or nil is passed in, which should never happen.

* Various tweaks to logging. This adjusts certain log statements to run log.Printf
  instead of calling log.Println(... fmt.Sprintf). The rest of the code and locations
  these are used will be unified in a follow up patch.
@staticfox staticfox force-pushed the change/refactor-cmd-and-template branch from 6b7a22c to 974b0d3 Compare June 13, 2024 20:24
This unifies the basic bootstrapping process when running any commands
that interact with the commotion directory.

This also cleans up the way that CleanupTerraformTemplatesDirectory
scrubs through the commotion directory as well.
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.

1 participant