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 support for pods (and some Ui refactoring) #468

Merged
merged 2 commits into from
Aug 4, 2020

Conversation

KKoukiou
Copy link
Contributor

@KKoukiou KKoukiou commented Jul 30, 2020

  • fix tests

@KKoukiou
Copy link
Contributor Author

@marusak @garrett I did not show yet the open ports and created time of the pod since the current mock up does not incoorporate them and I did not want to innovate too much.
I think it's fine that the pods attributes will me implemented in a followup.

@KKoukiou KKoukiou removed the no-test label Jul 31, 2020
@KKoukiou KKoukiou marked this pull request as ready for review August 3, 2020 07:59
@marusak
Copy link
Member

marusak commented Aug 3, 2020

List of TODOs: (I'll convert them into issues when this PR lands)

  • Create new pod
  • Pod actions (Delete, start/stop, pause/unpause)
  • Add container to pod
  • Add podless container
  • pod details (ID, name, status, created, owner, ports) (owner not needed for containers then)

Copy link
Member

@marusak marusak left a comment

Choose a reason for hiding this comment

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

This looks amazing, thank you!

I wrote down list of follow-ups, please feel free to edit this list. I'll convert it into issues once this lands.

My main problem is with sorting and filtering. It seems to be container-centric, which sometimes does not make much sense (especially with owner).

src/ContainerHeader.jsx Show resolved Hide resolved
test/check-application Outdated Show resolved Hide resolved
test/check-application Outdated Show resolved Hide resolved
src/Containers.jsx Outdated Show resolved Hide resolved
src/Containers.jsx Outdated Show resolved Hide resolved
src/Containers.jsx Show resolved Hide resolved
src/Containers.jsx Show resolved Hide resolved
src/Containers.jsx Show resolved Hide resolved
KKoukiou added a commit to KKoukiou/cockpit-podman that referenced this pull request Aug 3, 2020
@KKoukiou KKoukiou requested a review from marusak August 3, 2020 13:27
@garrett
Copy link
Member

garrett commented Aug 3, 2020

OK, so I've fixed the page's height (so it always stretches), fixed the stickiness of the filter bar, adjusted the spacing of the pods in all the scenarios I've encountered, and even included a Star Wars reference as a joke (that will be lost when this is squashed down 😉).

@KKoukiou: I hope I didn't break anything in 628e995, when I removed a nested wrapper.

KKoukiou added a commit to KKoukiou/cockpit-podman that referenced this pull request Aug 3, 2020
@marusak
Copy link
Member

marusak commented Aug 4, 2020

Thank you @KKoukiou for the changes, now it works much better!
There seems to be some problem though:

$ podman pod ps
POD ID        NAME      STATUS   CREATED        # OF CONTAINERS  INFRA ID
88fb67086715  user_one  Running  3 minutes ago  3                ebcfad8f79be
a25674684d34  user_pod  Created  21 hours ago   1                9378c45561e0

So I have one rootless pod that is running and there are 2 running containers in it, but when I pick Show Only Running it disappears. Are you able to reproduce, or should I do it locally?

$ podman pod create --name=user_one
$ podman run -dit --pod user_one fedora bash
$ podman run -dit --pod user_one fedora bash
In UI switch to see only running.

EDIT: When I refresh the page it is fine. Is it possible that we don't recognize when pod state changes? (no event/no processed event)

@KKoukiou
Copy link
Contributor Author

KKoukiou commented Aug 4, 2020

$ podman pod create --name=user_one
$ podman run -dit --pod user_one fedora bash

Added a HACK in our code and opened an issue to podman.

Copy link
Member

@marusak marusak left a comment

Choose a reason for hiding this comment

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

Thank you!

@marusak marusak merged commit f1e7c9b into cockpit-project:master Aug 4, 2020
marusak pushed a commit that referenced this pull request Aug 4, 2020
@KKoukiou KKoukiou deleted the pods branch August 5, 2020 16:32
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.

3 participants