Skip to content
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

Add docker pause functionality #317

Merged
merged 3 commits into from
May 14, 2022

Conversation

mark2185
Copy link
Contributor

I'm not sure what to do about translations, though.

Copy link
Owner

@jesseduffield jesseduffield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jesseduffield
Copy link
Owner

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

Copy link
Owner

@jesseduffield jesseduffield left a comment

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 :)

@mark2185
Copy link
Contributor Author

mark2185 commented May 9, 2022

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..."?

@jesseduffield
Copy link
Owner

@mark2185 yep 'pausing' works (lowercase for consistency with other loading statuses)

@mark2185 mark2185 force-pushed the feature/docker-pause branch 3 times, most recently from 0c2730b to 3015560 Compare May 11, 2022 15:36
@mark2185
Copy link
Contributor Author

mark2185 commented May 12, 2022

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?

Copy link
Owner

@jesseduffield jesseduffield left a comment

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 {
Copy link
Owner

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

Copy link
Contributor Author

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()
Copy link
Owner

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not.

Copy link
Owner

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!

Copy link
Contributor Author

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()

Copy link
Owner

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

@jesseduffield jesseduffield merged commit 3abff08 into jesseduffield:master May 14, 2022
@jesseduffield
Copy link
Owner

nice work @mark2185

@mark2185 mark2185 deleted the feature/docker-pause branch May 14, 2022 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants