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.

Sign up for GitHub

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

Jump to bottom

frontend: Home: Show the origin of each cluster #2536

Merged
merged 1 commit into from
Nov 7, 2024
Merged

frontend: Home: Show the origin of each cluster #2536

merged 1 commit into from
Nov 7, 2024

Conversation

skoeva
Copy link
Contributor

@skoeva skoeva commented Nov 6, 2024 •
edited
Loading

This change introduces a new "Origin" column for the cluster list on the home page, displaying whether a cluster came from a kubeconfig, Headlamp, or a dynamic cluster, with references when available.

Fixes: #1632

Testing

  • Open Headlamp with 2+ active clusters
  • Ensure that the cluster list on the Home page includes the Origin column with respective origins

image

@skoeva skoeva added the frontend Issues related to the frontend label Nov 6, 2024
@skoeva skoeva self-assigned this Nov 6, 2024
@skoeva skoeva linked an issue Nov 6, 2024 that may be closed by this pull request
Show the origin of each cluster in the home view #1632
Closed
@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Nov 6, 2024
illume
illume reviewed Nov 7, 2024
View reviewed changes
Copy link
Collaborator

@illume illume left a comment

Choose a reason for hiding this comment

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

👍 thanks

Left a couple of notes...

frontend/src/components/App/Home/index.tsx Outdated Show resolved Hide resolved
frontend/src/components/App/Home/index.tsx Outdated Show resolved Hide resolved
illume
illume reviewed Nov 7, 2024
View reviewed changes
Copy link
Collaborator

@illume illume left a comment

Choose a reason for hiding this comment

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

I just noticed this part from the issue...

(if we could have the plugin name here or a string identifying who added it'd be great).

I'm not sure where to get that information from though?

frontend/src/components/App/Home/index.tsx Show resolved Hide resolved
frontend/src/components/App/Home/index.tsx Outdated Show resolved Hide resolved
@skoeva
Copy link
Contributor Author

skoeva commented Nov 7, 2024

I just noticed this part from the issue...

(if we could have the plugin name here or a string identifying who added it'd be great).

I'm not sure where to get that information from though?

Yeah this isn't stored anywhere. The cluster metadata only includes the source so there wouldn't be any way to infer what the relevant plugin name is or who added it.

illume
illume approved these changes Nov 7, 2024
View reviewed changes
Copy link
Collaborator

@illume illume left a comment •
edited
Loading

Choose a reason for hiding this comment

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

👍 thanks

(probably just the translation change should be done, the docs change is just a nit)

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Nov 7, 2024
@skoeva
frontend: Home: Show the origin of each cluster
6d0dc28 
This change introduces a new "Origin" column for the cluster list on the
home page, displaying whether a cluster came from a kubeconfig,
Headlamp, or a dynamic cluster, with references when available.

Fixes: #1632

Signed-off-by: Evangelos Skopelitis <eskopelitis@microsoft.com>
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Nov 7, 2024
@illume illume merged commit 27a9fc7 into main Nov 7, 2024
18 checks passed
@illume illume deleted the show-cluster-origin branch November 7, 2024 15:36
@joaquimrocha
Copy link
Collaborator

I just noticed this part from the issue...

(if we could have the plugin name here or a string identifying who added it'd be great).

I'm not sure where to get that information from though?

Yeah this isn't stored anywhere. The cluster metadata only includes the source so there wouldn't be any way to infer what the relevant plugin name is or who added it.

Right. We can check if it's coming from a kubeconfig or it's been dynamically added. We should add a way (not related to this PR) for the plugins to register what the origin of their dynamically added cluster should be.

joaquimrocha
joaquimrocha reviewed Nov 7, 2024
View reviewed changes
Copy link
Collaborator

@joaquimrocha joaquimrocha 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 a few things can be improved.

frontend/src/components/App/Home/index.tsx
function getOrigin(cluster: Cluster): string {
if (cluster.meta_data?.source === 'kubeconfig') {
const kubeconfigPath = process.env.KUBECONFIG ?? '~/.kube/config';
return t('translation|Kubeconfig') + ` (${kubeconfigPath})`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need to translate Kubeconfig. And the value here should be: kubeconfig:PATH_TO_FILE
Can you make another PR to fix this, since this one is merged?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change as well as the other one mentioned are done here: #2543

frontend/src/components/App/Home/index.tsx Show resolved Hide resolved
skoeva added a commit that referenced this pull request Nov 7, 2024
@skoeva
frontend: Home: Improve cluster origin labels
966111e 
This change improves on #2536 by adjusting the labels in the cluster
origin column of the Home page for clarity and accuracy.

Signed-off-by: Evangelos Skopelitis <eskopelitis@microsoft.com>
This was referenced Nov 7, 2024
Merged
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joaquimrocha joaquimrocha joaquimrocha left review comments

@illume illume illume approved these changes

Assignees

@skoeva skoeva

Labels
frontend Issues related to the frontend lgtm This PR has been approved by a maintainer size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Show the origin of each cluster in the home view
3 participants
@skoeva @joaquimrocha @illume