chore: rename and respect pipeline.deletionGracePeriodSeconds #2226
Conversation
Signed-off-by: Derek Wang <[email protected]>
Signed-off-by: Derek Wang <[email protected]>
whynowy
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2226 +/- ##
==========================================
- Coverage 64.07% 63.87% -0.21%
==========================================
Files 338 338
Lines 41136 41142 +6
==========================================
- Hits 26358 26279 -79
- Misses 13712 13795 +83
- Partials 1066 1068 +2 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
|
@whynowy When we are doing a rolling update to a pipeline, we do a phase paused and then deletion right? |
Let's not make it complicated, but let the upper layer make the decision about how to utilize those timeout settings. |
| return *p.Spec.Lifecycle.DeprecatedDeleteGracePeriodSeconds | ||
| } | ||
| if p.DeletionGracePeriodSeconds != nil { | ||
| return *p.DeletionGracePeriodSeconds | ||
| } | ||
| return 30 |
There was a problem hiding this comment.
Could we make this as a constant
| return *lc.PauseGracePeriodSeconds | ||
| func (p Pipeline) GetPauseGracePeriodSeconds() int64 { | ||
| if p.Spec.Lifecycle.PauseGracePeriodSeconds != nil { | ||
| return *p.Spec.Lifecycle.PauseGracePeriodSeconds | ||
| } | ||
| return 30 |
There was a problem hiding this comment.
Could we make this as a constant
There was a problem hiding this comment.
Not a big fan of making it as a constant if it's only used in one place - considering there's some other things need to be updated (but could not use the constant, // +kubebuilder:default=30) when we want to make a change. In that case, prefer to keep them as close as possible. No strong opinion though.
|
@whynowy +1 for not adding the complexity layer of checking here. |
Signed-off-by: Derek Wang <[email protected]>
deletionGracePeriodSecondsas a common practice inspec.lifecycleand fix the doc.pipeline.deletionGracePeriodSecondsas well.