defaults: Add Rollback defaults #2080
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2080 +/- ##
==========================================
+ Coverage 54.07% 54.07% +<.01%
==========================================
Files 95 95
Lines 17293 17291 -2
==========================================
- Hits 9351 9350 -1
- Misses 6863 6867 +4
+ Partials 1079 1074 -5Continue to review full report at Codecov.
|
| if spec.Rollback == nil { | ||
| spec.Rollback = Service.Rollback.Copy() | ||
| } else { | ||
| if spec.Rollback.Monitor == nil { |
There was a problem hiding this comment.
else if {
.....
}
How about that?
There was a problem hiding this comment.
I'm thinking we may add other fields that need to be handled here in the future. Right now there is only one, but putting it inside an else structures the code so that another one can be added along side it (even if it looks weird right now).
| } | ||
|
|
||
| parallelism := int(defaults.Service.Update.Parallelism) |
There was a problem hiding this comment.
In the previous code, it seemed like at this point the parallelism and monitoring period came from the current value of updateConfig, I guess because it could possible come from service.Spec.Rollback or service.Spec.Update? Should we set this to:
parallelism := int(updateConfig.Parallelism)
monitoringPeriod, err = gogotypes.DurationFromProto(updateConfig.Monitor)
if err != nil {
monitoringPeriod, _ = gogotypes.DurationFromProto(defaults.Service.Update.Monitor)
}
instead?
There was a problem hiding this comment.
Ugh, yes, that was supposed to be updateConfig.
aaronlehmann
force-pushed
the
rollback-defaults
branch
from
fbbd769
to
82c4355
Compare
|
Just double-checking - by the time the service gets to the updater, it will be interpolated already with the defaults? |
|
No, interpolation is never used internally. It's an option for clients retrieving the service. See #1744. |
|
@aaronlehmann Ah ok, thanks. |
| } | ||
|
|
||
| parallelism := int(updateConfig.Parallelism) | ||
| monitoringPeriod, _ := gogotypes.DurationFromProto(updateConfig.Monitor) |
There was a problem hiding this comment.
Do we need to check this for nil, since this value could have come from service.Spec.Rollback or service.Spec.Update, which may not have been interpolated with defaults?
There was a problem hiding this comment.
You're absolutely correct. Sorry for all the defects.
There was a problem hiding this comment.
Not at all, that is the purpose of reviews :)
aaronlehmann
force-pushed
the
rollback-defaults
branch
from
82c4355
to
4206c00
Compare
Right now, the updater uses Update defaults for a rollback if no Rollback settings are provided in the service. Make the Rollback defaults explicit instead. Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
aaronlehmann
force-pushed
the
rollback-defaults
branch
from
4206c00
to
b6a4260
Compare
|
LGTM |
|
Thanks |
Right now, the updater uses
Updatedefaults for a rollback if noRollbacksettings are provided in the service. Make theRollbackdefaults explicit instead.