Skip to content

Show target cluster in the list of apps #1115

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
+5 −0
Open

Show target cluster in the list of apps #1115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions config/crds.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -734,6 +734,10 @@ spec:
jsonPath: .status.deploy.startedAt
name: Since-Deploy
type: date
- description: Target Cluster name taken from the kubeconfig.
jsonPath: .spec.cluster.kubeconfigSecretRef.name
name: TargetCluster
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
name: TargetCluster
name: Target-Cluster

Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to have a default value, something like <none> (if possible)? Or is it okay to have an empty column when target cluster is not used?
cc @neil-hickey @100mik What are your thoughts?

Copy link

Choose a reason for hiding this comment

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

Default ideally is the name of "this" cluster. i.e the one it's running on.

Copy link
Member

Choose a reason for hiding this comment

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

^ Makes sense 💯 , it would be good to have the target-cluster populated with the current cluster by default, but, is .spec.cluster.kubeconfigSecretRef.name populated by default?

Copy link
Author

Choose a reason for hiding this comment

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

This would require some code changes or it's about adding something like // +kubebuilder:default:="this" ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I imagine it would involve adding a cluster field to the status which is populated on reconcile. I do not imagine the controller populates any of the spec fields 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I definitely agree with Neil in regards to what should the default value be though!

type: string
- description: Time since creation
jsonPath: .metadata.creationTimestamp
name: Age
Expand Down
1 change: 1 addition & 0 deletions pkg/apis/kappctrl/v1alpha1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
// +kubebuilder:resource:categories={carvel}
// +kubebuilder:printcolumn:name=Description,JSONPath=.status.friendlyDescription,description=Friendly description,type=string
// +kubebuilder:printcolumn:name=Since-Deploy,JSONPath=.status.deploy.startedAt,description=Last time app started being deployed. Does not mean anything was changed.,type=date
// +kubebuilder:printcolumn:name=TargetCluster,JSONPath=.spec.cluster.kubeconfigSecretRef.name,description=Target Cluster name taken from the kubeconfig.,type=string
// +kubebuilder:printcolumn:name=Age,JSONPath=.metadata.creationTimestamp,description=Time since creation,type=date
// +protobuf=false
// An App is a set of Kubernetes resources. These resources could span any number of namespaces or could be cluster-wide (e.g. CRDs). An App is represented in kapp-controller using a App CR.
Expand Down