Resource owner now shown in every pane #6237

marcosdiez · 2021-07-07T13:09:49Z

I am not sure if this is useful anywhere but in a ReplicaSet and in a Job started by a CronJob Also, we have the "problem" of having the info shown twice in Pods.

k8s-ci-robot · 2021-07-07T13:09:59Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: marcosdiez
To complete the pull request process, please assign jeefy after the PR has been reviewed.
You can assign the PR to them by writing /assign @jeefy in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

maciaszczykm · 2021-07-09T09:30:40Z

src/app/frontend/common/components/objectmeta/template.html

+        <div key
+             i18n>Owner</div>
+        <div value
+             *ngFor="let ownerReference of objectMeta?.ownerReferences">


Handling for multiple owner refs is implemented here but I wonder how the view will look like if there will be more than one owner. Can you paste some screenshot?

I'd maybe rely on the controlled by card component that we use i.e. on pod details and try to reuse it for other resources.

Hi @maciaszczykm,

From my understanding, one can't automatically get two owners, but one can manually set them. This is how you get two of them.

I did it out of curiosity and I got this:

But I think @floreks is right. It's better to ditch this and just put the controlled by card everywhere.

I am thinking again. Do we really need the controller card ? Currently it's just used on the pods page, which has already too much content. Maybe removing it would be better. Just thinking outloud.

I'd still keep it but focus on reorganizing information on the detail pages overall. Use tabs, maybe use some smaller cards that we could put in a single row instead of always using 1 card per row.

@floreks ^^^^

Having this kind of generic resource owner information and creating internal links based on name/kind is not a good idea since we do not support every potential resource that might be created this way and it will result in dead/not working links inside the Dashboard.

We should support this explicitly on a per resource basis, similar to what we do for pods.

@floreks you are right. I just added the code. I took the liberty of copying what you did in the "events" pane.

Event list has it's own logic to avoid this kind of situations.

dashboard/src/app/frontend/common/components/resourcelist/event/component.ts

Lines 82 to 88 in 37f5f5d

getObjectHref(kind: string, name: string, namespace: string): string {

if (!Object.values(Resource).includes(kind.toLowerCase() as Resource)) {

return '';

}

return this.kdState_.href(kind.toLowerCase(), name, namespace);

}

@floreks added that code as well. thank you

codecov · 2021-09-27T13:32:17Z

Codecov Report

Merging #6237 (37f5f5d) into master (1cadbf5) will decrease coverage by 1.69%.
The diff coverage is n/a.

❗ Current head 37f5f5d differs from pull request most recent head a8225d6. Consider uploading reports for the commit a8225d6 to get more accurate results

@@            Coverage Diff             @@
##           master    #6237      +/-   ##
==========================================
- Coverage   44.22%   42.53%   -1.70%     
==========================================
  Files          46      217     +171     
  Lines        1282    10627    +9345     
  Branches      179      160      -19     
==========================================
+ Hits          567     4520    +3953     
- Misses        677     5841    +5164     
- Partials       38      266     +228

k8s-ci-robot · 2021-09-27T13:33:33Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

marcosdiez · 2021-10-28T18:12:14Z

@floreks Hi! What's needed for this PR to be merged now?

marcosdiez · 2021-11-04T13:26:28Z

@floreks @maciaszczykm Is there anything I can do for this code to be merged ?

floreks · 2021-11-04T14:29:44Z

My original concerns still exist. I'd prefer to have a separate controlled by card that would be added on a per resource basis since there are resources that do not have owner.

Also, we should not display multiple levels of ownership, i.e. pod is controlled by the replica set and replica set is controlled by the deployment. I wouldn't want to see both replica set and deployment (owner of an owner) on the pod details.

We also should not display raw owner information <kind>/<name> (raw string) without any additional information if we do not support such resource in the Dashboard.

marcosdiez · 2021-11-10T13:50:02Z

@floreks I am not sure I understand your concerns:

if a resource does not have an owner, the Owner field in Metadata is not shown at all.
we do not display multiple levels of ownership unless the metadata contains that. The only object I've ever seen with two owners is the one I manually modified the YAML to see how the UI would behave so I could answer @maciaszczykm 's question. (which is something legal but usually unnecessary per https://kubernetes.io/docs/concepts/overview/working-with-objects/owners-dependents/ . So unless you manually modify your pod, you won't see the Deployment, only the replicaset.
I personally prefer showing raw owner information <kind>/<name> instead of omitting information. More information is better then omitting information. It's also a nice hint that I should implement another pane :)

Think about it. Even though this is not your dream, the code works, it's helpful, does not consume many resources nor UI space. The kubernetes dashboard is better with it.

marcosdiez · 2022-02-01T12:44:33Z

Hello @floreks !

I am hoping you changed your mind and now thinks this PR is good to be merged.
An extra card would have to be created per resource and is not as future proof.

Please consider merging this PR as is!

Marcos

k8s-triage-robot · 2022-05-10T19:49:26Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

we only create owner links if we support the object getObjectHref now verifies if a resource really exists

marcosdiez · 2022-05-10T20:52:27Z

/remove-lifecycle stale

marcosdiez · 2022-07-07T13:08:33Z

@maciaszczykm @floreks Happy Birthday to this pull request!

k8s-ci-robot · 2022-08-13T04:52:36Z

@marcosdiez: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

floreks · 2022-09-23T08:08:33Z

Since #7047 has been merged and it introduces major architecture changes we should close this PR. If you are willing to work on it, you can open a new PR against the current master.

/close

k8s-ci-robot · 2022-09-23T08:08:39Z

@floreks: Closed this PR.

In response to this:

Since #7047 has been merged and it introduces major architecture changes we should close this PR. If you are willing to work on it, you can open a new PR against the current master.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 7, 2021

k8s-ci-robot requested review from chenrui333 and helight July 7, 2021 13:09

k8s-ci-robot added the language/fr Updates or issues for French translations. label Jul 7, 2021

maciaszczykm reviewed Jul 9, 2021

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 9, 2021

k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 27, 2021

k8s-ci-robot removed the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 27, 2021

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Sep 27, 2021

marcosdiez force-pushed the resource_owner branch from c8e9f89 to 5f58dc1 Compare September 27, 2021 13:35

marcosdiez force-pushed the resource_owner branch from 5f58dc1 to 04e1b46 Compare September 30, 2021 17:55

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 30, 2021

marcosdiez force-pushed the resource_owner branch from 04e1b46 to c7addeb Compare October 14, 2021 12:23

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2021

marcosdiez force-pushed the resource_owner branch from c7addeb to f404d37 Compare October 15, 2021 11:19

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 20, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 2, 2021

marcosdiez force-pushed the resource_owner branch from 9d83575 to 75d8629 Compare December 7, 2021 13:12

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 7, 2021

marcosdiez force-pushed the resource_owner branch from 75d8629 to bb33cba Compare January 31, 2022 10:32

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 31, 2022

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 9, 2022

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 10, 2022

marcosdiez added 2 commits May 10, 2022 17:47

resource owner

1c9aa2e

we only create owner links if we support the object getObjectHref now verifies if a resource really exists

i18n

a8225d6

marcosdiez force-pushed the resource_owner branch from bb33cba to a8225d6 Compare May 10, 2022 20:51

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 10, 2022

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 10, 2022

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 13, 2022

k8s-ci-robot closed this Sep 23, 2022

marcosdiez mentioned this pull request Sep 29, 2022

Resource owner now shown in every pane #7489

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resource owner now shown in every pane #6237

Resource owner now shown in every pane #6237

marcosdiez commented Jul 7, 2021 •

edited

Loading

k8s-ci-robot commented Jul 7, 2021

maciaszczykm Jul 9, 2021

floreks Jul 9, 2021

marcosdiez Jul 9, 2021

marcosdiez Jul 14, 2021

floreks Jul 15, 2021

marcosdiez Oct 11, 2021

floreks Oct 14, 2021 •

edited

Loading

marcosdiez Oct 14, 2021 •

edited

Loading

floreks Oct 15, 2021

marcosdiez Oct 15, 2021

codecov bot commented Sep 27, 2021 •

edited

Loading

k8s-ci-robot commented Sep 27, 2021

marcosdiez commented Oct 28, 2021

marcosdiez commented Nov 4, 2021

floreks commented Nov 4, 2021

marcosdiez commented Nov 10, 2021

marcosdiez commented Feb 1, 2022

k8s-triage-robot commented May 10, 2022

marcosdiez commented May 10, 2022

marcosdiez commented Jul 7, 2022

k8s-ci-robot commented Aug 13, 2022

floreks commented Sep 23, 2022

k8s-ci-robot commented Sep 23, 2022

	getObjectHref(kind: string, name: string, namespace: string): string {
	if (!Object.values(Resource).includes(kind.toLowerCase() as Resource)) {
	return '';
	}

	return this.kdState_.href(kind.toLowerCase(), name, namespace);
	}

Resource owner now shown in every pane #6237

Resource owner now shown in every pane #6237

Conversation

marcosdiez commented Jul 7, 2021 • edited Loading

k8s-ci-robot commented Jul 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

floreks Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

marcosdiez Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 27, 2021 • edited Loading

Codecov Report

k8s-ci-robot commented Sep 27, 2021

marcosdiez commented Oct 28, 2021

marcosdiez commented Nov 4, 2021

floreks commented Nov 4, 2021

marcosdiez commented Nov 10, 2021

marcosdiez commented Feb 1, 2022

k8s-triage-robot commented May 10, 2022

marcosdiez commented May 10, 2022

marcosdiez commented Jul 7, 2022

k8s-ci-robot commented Aug 13, 2022

floreks commented Sep 23, 2022

k8s-ci-robot commented Sep 23, 2022

marcosdiez commented Jul 7, 2021 •

edited

Loading

floreks Oct 14, 2021 •

edited

Loading

marcosdiez Oct 14, 2021 •

edited

Loading

codecov bot commented Sep 27, 2021 •

edited

Loading