Add docker pause functionality by mark2185 · Pull Request #317 · jesseduffield/lazydocker (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation15 Commits3 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
I'm not sure what to do about translations, though.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Actually I've just tested this out locally and looks like we're missing a loading state when pausing. Also, would we be able to add a similar keybinding for the services view so that it's consistent with the containers view? The services view appears when running lazydocker in a docker compose project
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changing to 'changes requested' for my own bookkeeping. Lemme know if you need any pointers :)
would we be able to add a similar keybinding for the services view so that it's consistent with the containers view
We sure could! I'll also add the required test files for future development since I don't usually use docker-compose
The loading state when pausing is referred to something like "Pausing..."?
@mark2185 yep 'pausing' works (lowercase for consistency with other loading statuses)
I walked back on adding necessary test files since the change was simple and I don't think I'll use docker-compose in the short term so I'd have to read up on it, and I don't really feel like it. Could you give it a spin?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, just a couple things. Happy to test upon next review
| return nil |
|---|
| } |
| return gui.WithWaitingStatus(gui.Tr.PausingStatus, func() error { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd extract this part out into its own function and call that from handleServicePause so that we're not duplicating the logic
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed it in a new commit so when you find the time you can easily test out the automatic recognizing of pausedness (I know it's not a word, but I do think it should be, haters be damned).
| return gui.createErrorPanel(gui.g, err.Error()) |
|---|
| } |
| return gui.refreshContainersAndServices() |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd see how it goes without this line here. Lazydocker should automatically recognise that the container is paused and then refresh accordingly
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done some digging and this looks to be an actual bug in the docker package where we refetch container info after docker tells us there's been a change but the change doesn't come through. Crazy!
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, now that we have the murderer, what are we going to do with it? The victim is apparently fine with gui.refreshContainersAndServices()
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah let's just stick to calling refreshContainersAndServices and we can look into the deeper issue in another PR
2 participants
