Add pausing deployment rollouts #838
Conversation
internal/pkg/handler/upgrade.go
Outdated
|
|
||
| time.AfterFunc(pauseDuration, func() { | ||
| deployment, err := clients.KubernetesClient.AppsV1().Deployments(itemNamespace).Get(context.TODO(), itemName, metav1.GetOptions{}) | ||
| if err != nil { |
There was a problem hiding this comment.
seven levels of if clauses - decrease the cyclomatic complexity
There was a problem hiding this comment.
@karl-johan-grahn Thanks for the review. The ode is really not that good. As I wrote in the issue this was just code to prove that it works. Would it be ok to add such a feature in general ? I would then of course refactor the code and add appropriate tests.
There was a problem hiding this comment.
@karl-johan-grahn I refactored the code in general:
- Reduced complexity / readability in general
- Put the code into a separate file instead of upgrade.go (separation of concerns)
- Reduced the number of changes introduced in PerformAction
- Refactor annotation to flags.go
- Add test cases for annotation and environment variable reload strategy
Could you please have a look at the changes ? Thanks in advance.
There was a problem hiding this comment.
Pull requests are very much welcome! But we will not add any temporary or work-in-progress changes, so you need to work on it until it is production-ready. Thousands of customers use the tool in production so it needs to go through thorough work before being merged.
There was a problem hiding this comment.
@karl-johan-grahn Of course. I would just need a little review / hints what might still be enhanced. Do you have any suggestions ?
There was a problem hiding this comment.
We will review it and you have to be a little patient, we only spend around 1 h each week on all open source repositories
karl-johan-grahn
changed the title
tom1299
force-pushed
the
add_pause_rollout
branch
from
4f4dda5 to
8359d87
Compare
tom1299
changed the title
Note that this pull request is just a draft for implementing the functionality described in the enhancement #837