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
|
Any chance on getting this reviewed and merged? This feature will be super helpful :) Thanks! |
tom1299
force-pushed
the
add_pause_rollout
branch
from
226a67a to
52be3b7
Compare
tom1299
force-pushed
the
add_pause_rollout
branch
from
52be3b7 to
785e7bb
Compare
|
@galkindmitrii @karl-johan-grahn I would be happy to continue working on this. I made some smaller refactorings but would now say it could be reviewed. Regarding documentation I am not sure where I should add it. |
tom1299
force-pushed
the
add_pause_rollout
branch
from
febde70 to
6f6886d
Compare
|
@karl-johan-grahn I was wondering if we might still be able to get this pull request merged. I've taken some time to rebase the branch on master, refactored the code to support patching resources, moved most of the logic into a dedicated Go file, and added some additional tests. |
Note that this pull request is just a draft for implementing the functionality described in the enhancement #837