feat: update plugin to check for a minimum required version #121
Conversation
|
We have created an issue in Pivotal Tracker to manage this: https://www.pivotaltracker.com/story/show/186780945 The labels on this github issue will be updated when the story is started. |
internal/upgrader/upgrader.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| return performCheckUpToDate(instances, log) |
There was a problem hiding this comment.
Doesn't actually check that anything is up to date, so change to a more meaningful name.
There was a problem hiding this comment.
@blgm Thank you very much for reviewing the code.
After analyzing the alternatives, I preferred removing the method instead of changing the name of a dummy method. It turned out that the code is now more readable, but I had to repeat the dryRun method call in the switch cases case cfg.MinVersionRequired != "" and cfg.FailIfNotUpToDate. In my opinion, this will help us in understanding the logic and in the next refactor, where we could improve the dryRun functionality. The logging strategy seems inconsistent. For example, in performDryRun, it logs upgrade failures for each service instance, which might be misleading as it's just a dry run.
Due to the fact that we have already documented the `check-up-to-date` flag, we would prefer to maintain the name to avoid changes in our documentation and to prevent any potential impact on customers from our change. [#186528305](https://www.pivotaltracker.com/story/show/186528305)
By centralizing the validation, we avoid incorrect usages of the method and repetition of the check if the version is an empty value. We also return a string which allows us to assign the value directly to the configuration property. [#186528305](https://www.pivotaltracker.com/story/show/186528305)
The method `performCheckUpToDate` is intended to execute an action when there are instances not up-to-date. It turned out that the method was a dummy method with minimal functionality. The name of the method was causing problems in understanding the logic. We prefer to remove it now and perform a refactor later to improve the modularity of the code. [#186528305](https://www.pivotaltracker.com/story/show/186528305)
internal/config/config.go
Outdated
| @@ -27,42 +28,34 @@ func ParseConfig(conn CLIConnection, args []string) (Config, error) { | |||
| flagSet.IntVar(&cfg.ParallelUpgrades, parallelFlag, parallelDefault, parallelDescription) | |||
| flagSet.BoolVar(&cfg.HTTPLogging, httpLoggingFlag, httpLoggingDefault, httpLoggingDescription) | |||
| flagSet.BoolVar(&cfg.DryRun, dryRunFlag, dryRunDefault, dryRunDescription) | |||
| flagSet.BoolVar(&cfg.CheckUpToDate, checkUpToDateFlag, checkUpToDateDefault, checkUpToDateDescription) | |||
| flagSet.BoolVar(&cfg.CheckUpToDate, checkUpToDateFlag, checkUpToDateDefault, checkToDateDescription) | |||
There was a problem hiding this comment.
typo: checkToDateDescription -> checkUpToDateDescription
zucchinidev
changed the title
Checklist: