github.com/lukasheimann/cloudfoundrycli@v7.1.0+incompatible/doc/adr/0004-v7-push-refactor.md (about)

     1  # 4. V7 `cf push` Refactor
     2  
     3  Date: 2019-07-02
     4  
     5  ## Status
     6  
     7  Accepted
     8  
     9  ## Context
    10  
    11  _The issue motivating this decision, and any context that influences or constrains the decision._
    12  
    13  As the V3 Acceleration Team was developing the V7 version of `cf push`, the team saw opportunities to structure the code to make it easier to understand and maintain. In general, the `push` command logic is some of the most complex in the codebase, and this command was still considered experimental, so it seemed like the right time and place to invest our energy in refactoring.
    14  
    15  In the V6 CLI, the code for `push` was split roughly into two methods: `Conceptualize` and `Actualize`. `Conceptualize` was responsible for taking user input (flags, manifest properties, etc) and generating a "push plan" for each app that is being pushed (a struct containing various pieces of information needed to create/push/update the app). `Actualize` was responsible for taking these push plans and, based on the plans, taking the necessary actions to complete the push process. The refactor preserves the spirit of this division, but prevents these two methods from growing too large and unmaintainable.
    16  
    17  Some of the goals of the refactor were:
    18  
    19  - Make it easier to add features to `push` (e.g. new flags, new options for flags, new manifest properties)
    20  - Make it easier to unit test the components of the `push` workflow. This means meant splitting the command into several smaller functions that can be tested individually but composed into sequences based on the given user input.
    21  
    22  ## Decision
    23  
    24  _The change that we're proposing or have agreed to implement._
    25  
    26  Both the `Conceptualize` and `Actualize` parts of the push process have been split up and refactored in a manner inspired by [hexagonal architecture](https://medium.com/@nicolopigna/demystifying-the-hexagon-5e58cb57bbda).
    27  
    28  ### Hexagonal architecture
    29  
    30  For our purposes, "hexagonal architecture" means the following:
    31  
    32  - There is one place in the code responsible for calling a given sequence of functions in order. The output of one function is passed in as input to the next function.
    33  - All of these functions have the same signature.
    34  - The central place where the functions are called is agnostic to what the functions are doing.
    35  - Any new features/branches/logic should be added to one or more of these functions (or, a new function should be added to encapsulate it). We generally shouldn't need to touch the place where the functions are called.
    36  
    37  ### Splitting up `Conceptualize`
    38  
    39  What was previously called `Conceptualize` now looks roughly like this:
    40  
    41  _Note: We are still iterating on this code, so it may evolve over time and no longer look exactly like this. Still, the code below illustrates the idea we were going for._
    42  
    43  ```go
    44  var pushPlans []PushPlan
    45  
    46  for _, manifestApplication := range getEligibleApplications(parser, appNameArg) {
    47      plan := PushPlan{
    48          OrgGUID:   orgGUID,
    49          SpaceGUID: spaceGUID,
    50      }
    51  
    52      for _, updatePlan := range actor.PreparePushPlanSequence {
    53          var err error
    54          plan, err = updatePlan(plan, overrides, manifestApplication)
    55          if err != nil {
    56              return nil, err
    57          }
    58      }
    59  
    60      pushPlans = append(pushPlans, plan)
    61  }
    62  
    63  return pushPlans, nil
    64  ```
    65  
    66  This is the central place where all of the "prepare push plan" functions are called. We loop through `actor.PreparePushPlanSequence`, an array of functions, and call each one with a push plan. They each have the opportunity to **return a modified push plan**, which then gets **passed into the next function**.
    67  
    68  In this case, each function is also called with `overrides` and `manifestApplication`, which represent user input (in the form of flags and manifest properties, respectively). This lets each function inspect the user input and modify the push plan accordingly.
    69  
    70  When this loop completes, the original push plan has flowed through each function in the sequence and has been modified to include information based on the given flags/manifest.
    71  
    72  Let's now look at how `actor.PreparePushPlanSequence` is defined:
    73  
    74  ```go
    75  actor.PreparePushPlanSequence = []UpdatePushPlanFunc{
    76      SetupApplicationForPushPlan,
    77      SetupDockerImageCredentialsForPushPlan,
    78      SetupBitsPathForPushPlan,
    79      SetupDropletPathForPushPlan,
    80      actor.SetupAllResourcesForPushPlan,
    81      SetupDeploymentStrategyForPushPlan,
    82      SetupNoStartForPushPlan,
    83      SetupNoWaitForPushPlan,
    84      SetupSkipRouteCreationForPushPlan,
    85      SetupScaleWebProcessForPushPlan,
    86      SetupUpdateWebProcessForPushPlan,
    87  }
    88  ```
    89  
    90  This is just a simple array with a bunch of functions that all conform to the correct interface (they all have type `UpdatePushPlanFunc`).
    91  
    92  Here is an example of one of them:
    93  
    94  ```go
    95  func SetupScaleWebProcessForPushPlan(pushPlan PushPlan, overrides FlagOverrides, manifestApp manifestparser.Application) (PushPlan, error) {
    96  	if overrides.Memory.IsSet || overrides.Disk.IsSet || overrides.Instances.IsSet {
    97  		pushPlan.ScaleWebProcessNeedsUpdate = true
    98  
    99  		pushPlan.ScaleWebProcess = v7action.Process{
   100  			Type:       constant.ProcessTypeWeb,
   101  			DiskInMB:   overrides.Disk,
   102  			Instances:  overrides.Instances,
   103  			MemoryInMB: overrides.Memory,
   104  		}
   105  	}
   106  	return pushPlan, nil
   107  }
   108  ```
   109  
   110  This simple function populates fields on the push plan based on flag overrides and returns the enhanced push plan, making it easy to test on its own.
   111  
   112  When all of the functions have run and updated the push plan, the "actualize" step doesn't need to know about flags and manifest properties anymore; it only needs to receive a push plan because all user input has been resolved and combined into the push plan object.
   113  
   114  ### Splitting up `Actualize`
   115  
   116  There is still (at the time of this writing) a function called `Actualize`, but it now looks like this:
   117  
   118  ```go
   119  for _, changeAppFunc := range actor.ChangeApplicationSequence(plan) {
   120      plan, warnings, err = changeAppFunc(plan, eventStream, progressBar)
   121      warningsStream <- warnings
   122      if err != nil {
   123          errorStream <- err
   124          return
   125      }
   126      planStream <- plan
   127  }
   128  ```
   129  
   130  This is quite similar to the loop through `actor.PreparePushPlanSequence` above. We loop through `actor.ChangeApplicationSequence(plan)`, which returns an array of functions, and we call each one with the push plan. Each one returns a push plan that then gets passed in to the next function.
   131  
   132  _Note: The rest of that code uses streams to report progress, errors, and warnings, but this is not the focus of this ADR (and we may end up changing this as well)._
   133  
   134  The **biggest difference** from `Conceputalize` is that instead of being a static list of functions (like `actor.PreparePushPlanSequence`), `actor.ChangeApplicationSequence` is a function that takes in a push plan and returns an array of `ChangeApplicationFunc`s. This allows us to dynamically build up the sequence of actions we run based on the push plan, rather than run the same sequence every time.
   135  
   136  Let's look at how that works:
   137  
   138  ```go
   139  actor.ChangeApplicationSequence = func(plan PushPlan) []ChangeApplicationFunc {
   140      var sequence []ChangeApplicationFunc
   141      sequence = append(sequence, actor.GetUpdateSequence(plan)...)
   142      sequence = append(sequence, actor.GetPrepareApplicationSourceSequence(plan)...)
   143      sequence = append(sequence, actor.GetRuntimeSequence(plan)...)
   144      return sequence
   145  }
   146  ```
   147  
   148  This function is responsible for building up the sequence based on the given plan. It delegates to three helpers, each of which builds up a subsequence of actions. Here's one of them:
   149  
   150  ```go
   151  func ShouldCreateBitsPackage(plan PushPlan) bool {
   152  	return plan.DropletPath == "" && !plan.DockerImageCredentialsNeedsUpdate
   153  }
   154  
   155  // ...
   156  
   157  func (actor Actor) GetPrepareApplicationSourceSequence(plan PushPlan) []ChangeApplicationFunc {
   158  	var prepareSourceSequence []ChangeApplicationFunc
   159  	switch {
   160  	case ShouldCreateBitsPackage(plan):
   161  		prepareSourceSequence = append(prepareSourceSequence, actor.CreateBitsPackageForApplication)
   162  	case ShouldCreateDockerPackage(plan):
   163  		prepareSourceSequence = append(prepareSourceSequence, actor.CreateDockerPackageForApplication)
   164  	case ShouldCreateDroplet(plan):
   165  		prepareSourceSequence = append(prepareSourceSequence, actor.CreateDropletForApplication)
   166  	}
   167  	return prepareSourceSequence
   168  }
   169  ```
   170  
   171  In this case, we only want to include one of these three functions in the final sequence, which is determined based on properties of the push plan.
   172  
   173  Since all of these functions are small and straightforward on their own, they are easy to unit test. They can be **composed together in different sequences to build up different push workflows** (based on different flags/manifests), and this is where the refactor really starts to pay off.
   174  
   175  ## Consequences
   176  
   177  _What becomes easier or more difficult to do and any risks introduced by the change that will need to be mitigated._
   178  
   179  ### What becomes easier?
   180  
   181  Figuring out where to write the code to add a new flag becomes easier. Consider [this recent commit](https://github.com/cloudfoundry/cli/commit/9dbddd165ae77dbc33e7dc34ae896e6f880ce3ff), which added the `--no-wait` flag to the V7 `push` command. These were the bulk of the changes needed to add this new branch to the workflow:
   182  
   183  - [New, three-line method](https://github.com/cloudfoundry/cli/commit/9dbddd165ae77dbc33e7dc34ae896e6f880ce3ff#diff-8efa044476f78f1ca7dbe0f90addeca4) implementing the `UpdatePushPlanFunc` interface, plus [unit tests for it](https://github.com/cloudfoundry/cli/commit/9dbddd165ae77dbc33e7dc34ae896e6f880ce3ff#diff-03f194b5c186fb49de70b64082df2191)
   184  - [One-line change](https://github.com/cloudfoundry/cli/commit/9dbddd165ae77dbc33e7dc34ae896e6f880ce3ff#diff-d84d0497f89a03505f051cbe0a418739) to the actor to add this new method to the `ChangeApplicationSequence`
   185  - [Simple change](https://github.com/cloudfoundry/cli/commit/9dbddd165ae77dbc33e7dc34ae896e6f880ce3ff#diff-59e4c63c96b86434900c067fc7d0e49f) to pass the new push plan property as into the methods that need it
   186  
   187  There are more changes in that commit, but that highlights the parts relevant to this ADR.
   188  
   189  ### What becomes harder?
   190  
   191  It is slightly harder to grasp how all these pieces fit together at first glance. For the arrays of functions detailed above, it is not immediately clear where or how they are called (since that is abstracted away into a different part of the codebase). We believe that after spending time understanding the new structure of things, developers will appreciate how straightforward it is to make changes.