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

Containers page: listing. #5258

Merged
merged 10 commits into from
Sep 20, 2023

Conversation

scures
Copy link
Contributor

@scures scures commented Jul 31, 2023

Addresses some points of #4506

We have some limitations since we don't have a whole API yet, basic actions are not possible (or I couldn't find a way) but we have the structure to enable them ready.

  • Display a comprehensive list of containers
  • Provide information such as container name, state, image, creation date, and ports
  • Display port mappings for containers

We can have follow-up tasks to implement more features (actions, for example) once available.

image

scures added 7 commits July 31, 2023 12:31
Signed-off-by: scures <scurescu@suse.com>
Signed-off-by: scures <scurescu@suse.com>
Signed-off-by: scures <scurescu@suse.com>
Signed-off-by: scures <scurescu@suse.com>
Signed-off-by: scures <scurescu@suse.com>
Signed-off-by: scures <scurescu@suse.com>
@gaktive gaktive requested a review from rak-phillip July 31, 2023 18:10
Copy link
Contributor

@rak-phillip rak-phillip left a comment

Choose a reason for hiding this comment

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

I gave this a quick review and left a few notes.

I was also able to crash the page using the following docker-compose.yaml (renamed to docker-compose.txt so I could upload) while I was testing.

docker-compose.txt

This is the error I received after running docker compose pull & docker compose -p penpot -f docker-compose.yaml up -d, and navigating to the Containers page

client.js?4497:103 TypeError: Cannot read properties of null (reading '0')
    at eval (Containers.vue?3246:164:1)
    at Array.forEach (<anonymous>)
    at VueComponent.getUniquePorts (Containers.vue?3246:162:1)
    at fn (Containers.vue?0334:29:1)
    at normalized (vue.runtime.esm.js?2b0e:2605:1)
    at Proxy.renderSlot (vue.runtime.esm.js?2b0e:2701:1)
    at eval (index.vue?dcca:573:1)
    at Proxy.renderList (vue.runtime.esm.js?2b0e:2646:1)
    at _vm._t.fullColspan (index.vue?dcca:571:1)
    at Proxy.renderSlot (vue.runtime.esm.js?2b0e:2706:1)

pkg/rancher-desktop/assets/translations/en-us.yaml Outdated Show resolved Hide resolved
pkg/rancher-desktop/pages/Containers.vue Outdated Show resolved Hide resolved
pkg/rancher-desktop/pages/Containers.vue Outdated Show resolved Hide resolved
pkg/rancher-desktop/pages/Containers.vue Outdated Show resolved Hide resolved
pkg/rancher-desktop/pages/Containers.vue Outdated Show resolved Hide resolved
Signed-off-by: scures <scurescu@suse.com>
@scures
Copy link
Contributor Author

scures commented Aug 3, 2023

I gave this a quick review and left a few notes.

I was also able to crash the page using the following docker-compose.yaml (renamed to docker-compose.txt so I could upload) while I was testing.

docker-compose.txt

This is the error I received after running docker compose pull & docker compose -p penpot -f docker-compose.yaml up -d, and navigating to the Containers page

client.js?4497:103 TypeError: Cannot read properties of null (reading '0')
    at eval (Containers.vue?3246:164:1)
    at Array.forEach (<anonymous>)
    at VueComponent.getUniquePorts (Containers.vue?3246:162:1)
    at fn (Containers.vue?0334:29:1)
    at normalized (vue.runtime.esm.js?2b0e:2605:1)
    at Proxy.renderSlot (vue.runtime.esm.js?2b0e:2701:1)
    at eval (index.vue?dcca:573:1)
    at Proxy.renderList (vue.runtime.esm.js?2b0e:2646:1)
    at _vm._t.fullColspan (index.vue?dcca:571:1)
    at Proxy.renderSlot (vue.runtime.esm.js?2b0e:2706:1)

Hopefully with the new changes & improvements that won't happen again.

image

Copy link
Contributor

@rak-phillip rak-phillip left a comment

Choose a reason for hiding this comment

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

I think this is shaping up quite nicely, left a few minor comments 🙂

data() {
return {
settings: defaultSettings,
ddClient: null,
Copy link
Contributor

Choose a reason for hiding this comment

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

ddClient will be made reactive because it's a member of data. That doesn't feel right to me because I can't imagine we would really care about tracking changes to ddClient in this component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Any suggestion on what would be the other approach?

Copy link
Contributor

Choose a reason for hiding this comment

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

There are many ways to do this, but the generally accepted way is to simply assign a value to this.ddClient in the created hook.

vuejs/vue#1988 (comment)

I think that we can follow-up in a separate PR to make this change.

pkg/rancher-desktop/pages/Containers.vue Outdated Show resolved Hide resolved
pkg/rancher-desktop/router.ts Outdated Show resolved Hide resolved
@gaktive gaktive added this to the 1.11 milestone Aug 28, 2023
scures added 2 commits August 29, 2023 11:18
Signed-off-by: scures <scurescu@suse.com>
@scures
Copy link
Contributor Author

scures commented Sep 4, 2023

@rak-phillip, I sent a commit to reduce the length of the sha names & filter out the containers in the kube-system namespace.

image

@gaktive gaktive requested a review from rak-phillip September 19, 2023 17:13
Copy link
Contributor

@rak-phillip rak-phillip left a comment

Choose a reason for hiding this comment

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

I think what we have here looks like a good start to listing containers.

data() {
return {
settings: defaultSettings,
ddClient: null,
Copy link
Contributor

Choose a reason for hiding this comment

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

There are many ways to do this, but the generally accepted way is to simply assign a value to this.ddClient in the created hook.

vuejs/vue#1988 (comment)

I think that we can follow-up in a separate PR to make this change.

@rak-phillip rak-phillip merged commit ea032b8 into rancher-sandbox:main Sep 20, 2023
@rak-phillip rak-phillip mentioned this pull request Sep 20, 2023
@scures scures self-assigned this Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants