fix: reverse order of deployers during cleanup (#7284) #7925
Conversation
pkg/skaffold/deploy/deploy_mux.go
Outdated
| // Reverse order of deployers for cleanup to ensure resources | ||
| // are removed before their definitions are removed. | ||
| revDeployers := m.deployers | ||
| for i, j := 0, len(revDeployers)-1; i < j; i, j = i+1, j-1 { | ||
| revDeployers[i], revDeployers[j] = revDeployers[j], revDeployers[i] | ||
| } |
There was a problem hiding this comment.
Thanks. Was wondering if there is a utility method which already does that.
There was a problem hiding this comment.
That's a good point. Searching the internet I did not find any lib that does this. I was also supprised the slice type has no internal method since this is such a common task. Searching the skaffold source code for for i, j and for for .*,.*:=.*,len does not give any results.
The golang completion on the other side provides a slice.reverse! method which automatically creates the for loop. Maybe that's why there is no lib or function.
I can put such a function somewhere in the skaffold source code if you wish. I need help finding the location where such a function would be placed best.
There was a problem hiding this comment.
You can write this function here:
https://github.com/GoogleContainerTools/skaffold/blob/main/pkg/skaffold/util/util.go
There was a problem hiding this comment.
Looking into this now.
There was a problem hiding this comment.
@gsquared94 I tried adding a function to the file you suggested. However when I did this, I created a dependency lopp between util.go and deploy.go.
I also tried creating a generic slice reverse function in utils but since the go.mod file says to use at least go 1.17, I could not use generics.
In the end I added it to DeployMux since this is the only place using this function for now.
Codecov Report
@@ Coverage Diff @@
## main #7925 +/- ##
==========================================
- Coverage 70.48% 66.78% -3.70%
==========================================
Files 515 594 +79
Lines 23150 28773 +5623
==========================================
+ Hits 16317 19217 +2900
- Misses 5776 8136 +2360
- Partials 1057 1420 +363
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
chronicc
force-pushed
the
reverse-cleanup-order
branch
from
a1bbb01
to
179eddd
Compare
chronicc
force-pushed
the
reverse-cleanup-order
branch
from
179eddd
to
153ae88
Compare
|
Sorry for pushing again. I've rebased to main, fixed the linting erros and added a file to the gitignore for asdf. |
chronicc
force-pushed
the
reverse-cleanup-order
branch
2 times, most recently
from
ca20879
to
92229a2
Compare
.tool-versions is used by asdf (package manager) to set the go version for this repository
chronicc
force-pushed
the
reverse-cleanup-order
branch
from
92229a2
to
df4be15
Compare
Fixes: #7284
Related: Relevant tracking issues, for context
Merge before/after: Dependent or prerequisite PRs
Description
Add a new deployer slice during cleanup function of the DeployerMux which is reversed in it's order. This change should be covered by the existing unittests since no new function was added.
User facing changes (remove if N/A)
Before:
Modules are undeployed in the same order as they are deployed. Visible in the console as
After:
Modules are undeployed in reverse order.