Uh oh!
There was an error while loading. Please reload this page.
Create a dry run mode - #9
Conversation
squat
left a comment
There was a problem hiding this comment.
This implementation will not scale and will introduce bugs going forward. please refer to the comment for a description on the correct path forward regarding parameterizing functions for dry-runs. Also, please squash the dry-run commits so they are easier to review and separate the description length bug-fix into its own PR.
| } | ||
| if fields.Description != "" { | ||
| log.Infof(" Description: %s...", fields.Description[0:20]) | ||
| if len(fields.Description) > 20 { |
There was a problem hiding this comment.
These changes seem unrelated to the dry-run option. Separate this into it's own PR so we can track and revert dry-run changes separately from this bug fix.
| if err != nil { | ||
| log.Errorf("Error updating JIRA issue %s: %s", jIssue.Key, err) | ||
| defer res.Body.Close() | ||
| body, _ := ioutil.ReadAll(res.Body) |
| } | ||
| issue, res, err := jClient.Issue.Update(issue) | ||
| if !dryRun { |
There was a problem hiding this comment.
this will not scale well and will get unmanageable quite fast. the behavior of this function is now being drastically altered by a variable that is not a param. Testing this is messy and ensuring correctness becomes increasingly difficult to reason about. The correct way to do this is for all of these functions to be methods on a issue-sync config struct that encapsulates all the configuration, including dry-run. These methods then operate on a method receiver that holds all the information needed to control the operation of the function. Either that, or all of these methods take an additional parameter dryRun.
This functionality is now replicated in #15. We may be able to simply close this PR. Let me check... EDIT: No, I think it may just be easier to merge this anyway. There's a handful of changes here that do need to propagate upward. |
squat
left a comment
There was a problem hiding this comment.
This PR needs work. The dryRun option still uses if/else everywhere to determine what functionality to execute rather than two different structs that implement the same interface. I realize that is coming in #15, but that work should really just have been written in this PR. Also, all the configuration is floating around in global variables still. Lets get this merged quickly so we can get the fixes implemented ASAP. That will give us more sanity. Please take a look at the few comments and then lets merge.
| if fields.Description != "" { | ||
| fields.Description = newlineReplaceRegex.ReplaceAllString(fields.Description, "\\n") | ||
| if len(fields.Description) > 20 { | ||
| log.Infof(" Description: %s...", fields.Description[0:20]) |
There was a problem hiding this comment.
why do some of these have leading whitespace?
There was a problem hiding this comment.
The fields all have whitespace for clarity. All the dry-runs print:
Doing action
Field1: ...
Field2: ...
| if !dryRun { | ||
| req, err := jClient.NewRequest("PUT", fmt.Sprintf("rest/api/2/issue/%s/comment/%s", jIssue.Key, jComment.ID), request) | ||
| if err != nil { | ||
| log.Errorf("Error creating comment update request: %s", err) |
| project jira.Project | ||
| dryRun bool |
There was a problem hiding this comment.
this should be a field on a config struct, not a global variable.
MorganEPatch
commented
Aug 7, 2017
Okay. I just felt that that would be too much all in one PR, especially since it depended on the config struct as well, which is also its own big changes. |
When passing --dry-run or -d to the command, it will print out the list of actions that it would take, but will not send any create or update requests to the servers.
Depends on #8.
Creates a dry run mode which prints out the actions to be taken but does not create or update anything.