-
Notifications
You must be signed in to change notification settings - Fork 311
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
wait for dev pod to terminate #674
Conversation
Codecov Report
@@ Coverage Diff @@
## master #674 +/- ##
==========================================
+ Coverage 38.23% 38.59% +0.35%
==========================================
Files 42 43 +1
Lines 3588 3620 +32
==========================================
+ Hits 1372 1397 +25
- Misses 2136 2141 +5
- Partials 80 82 +2
Continue to review full report at Codecov.
|
cmd/down.go
Outdated
@@ -181,3 +194,14 @@ func stopSyncthing(dev *model.Dev) { | |||
log.Infof("failed to delete existing syncthing folder") | |||
} | |||
} | |||
|
|||
func waitForDevPodTermination(c kubernetes.Interface, p *v1.Pod) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we wait for all pods with label interactive.dev.okteto.com: dev.Name
or detached.dev.okteto.com: dev.Name
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, let me look into this. there are a couple other issues as well.
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
package down |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created this package to start moving logic outside of the main package, following kubectl
's pattern. I think it's easier if we leave the user-interaction stuff in main
(cmd parsing, param matching, input - output) and start moving the logic to packages like this.
pkg/cmd/down/wait.go
Outdated
|
||
wg := &sync.WaitGroup{} | ||
|
||
waitForDevPodsTermination(c, d.Namespace, interactive, wg, t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing calling it as a goroutine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
pkg/cmd/down/wait.go
Outdated
|
||
waitForDevPodsTermination(c, d.Namespace, interactive, wg, t) | ||
if len(d.Services) > 0 { | ||
waitForDevPodsTermination(c, d.Namespace, detached, wg, t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
for i := 0; i < t; i++ { | ||
ps, err := pods.ListBySelector(namespace, selector, c) | ||
if err != nil { | ||
log.Infof("failed to get dev pods with selector %s, exiting: %s", selector, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if not found I wouldn't log anything. Try running "okteto down" two times in a row
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pods.ListBySelector only returns an error if client-go returns an error. if there aren't any pods it's all fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I was looking to https://github.com/okteto/okteto/pull/674/files#diff-7d4bf51853e76e6affcb154348fe6cc0R46
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
Fixes #664
Proposed changes
okteto down