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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scopes: Adds kinds for browsing the scope node tree. #86975

Merged
merged 7 commits into from May 3, 2024
Merged

Conversation

bergquist
Copy link
Contributor

@bergquist bergquist commented Apr 26, 2024

This is my proposal for the Scope Node data model.

The ScopeNode is a tree that can render selected or non selectable items (decided by the isLeaf bool).

Having leafType and leafName allows the API to include both scopes, dashboards, apps or any other kind within the tree. While we will focus only on scopes right now we should use this for more things eventually.

leftName could have a better name that is more clear about referencing another entity identifier.

To find the root nodes in the tree I'd like to find all ScopeNode.ParentName that equals "" or nil depending on if the name can be a pointer or not. I think a pointer is a cleaner solution but Im not sure if it would be possible to filter based on it.

As for the API server part... I've just tried to copy as much as possible from the code I see for scope and scopedashboard 馃槅

@bergquist bergquist added no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels Apr 26, 2024
@grafana-delivery-bot grafana-delivery-bot bot added this to the 11.1.x milestone Apr 26, 2024
}

type ScopeNodeSpec struct {
ParentName string `json:"parentName"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally this would be a pointer so it can be nil. But when it was it caused from here: https://github.com/grafana/grafana/pull/86975/files#diff-2f14ede265a1a7398479d678d07da5e351ab585e62ef22dbadfcc1cde64e02e4R177

Copy link
Member

Choose a reason for hiding this comment

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

there are examples of using string pointers in k8s code, so it seems like it should be supported.

did you try something like this in SelectableScopeNodeFields?

if obj.Spec.ParentName == nil {
  return generic.ObjectMetaFieldsSet(&obj.ObjectMeta, false)
}

Copy link
Contributor Author

@bergquist bergquist Apr 30, 2024

Choose a reason for hiding this comment

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

Sorry for my lack of knowledge about the apiserver.. But what if I want to query for ScopeNodes with parentName == nil ? Would that work in such case?

That's how I envision finding the root nodes in the tree.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for my lack of knowledge about the apiserver

no need to apologize for not knowing something! i'm not sure of the correct approach here either 馃槃

maybe using an empty string would work?

func SelectableScopeNodeFields(obj *scope.ScopeNode) fields.Set {
        parentName := ""
        if obj.Spec.ParentName != nil {
          parentName = *obj.Spec.ParentName
        }
	return generic.MergeFieldsSets(generic.ObjectMetaFieldsSet(&obj.ObjectMeta, false), fields.Set{
		"spec.parentName": parentName,
	})
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yupp! that worked :)

@bergquist bergquist force-pushed the scopes_tree_node branch 2 times, most recently from 6aec13e to ca0ac61 Compare April 29, 2024 12:41
@bergquist bergquist marked this pull request as ready for review April 29, 2024 12:58
@bergquist bergquist requested a review from a team as a code owner April 29, 2024 12:58
func GetAttrs(obj runtime.Object) (labels.Set, fields.Set, error) {
if s, ok := obj.(*scope.Scope); ok {
return labels.Set(s.Labels), SelectableScopeFields(s), nil
}
if s, ok := obj.(*scope.ScopeDashboardBinding); ok {
return labels.Set(s.Labels), SelectableScopeDashboardBindingFields(s), nil
}
if s, ok := obj.(*scope.ScopeNode); ok {
Copy link
Member

Choose a reason for hiding this comment

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

up to you, but this might be less verbose if it was refactored to a type switch now that there are three types

Signed-off-by: bergquist <carl.bergquist@gmail.com>
Signed-off-by: bergquist <carl.bergquist@gmail.com>
Signed-off-by: bergquist <carl.bergquist@gmail.com>
Signed-off-by: bergquist <carl.bergquist@gmail.com>
Signed-off-by: bergquist <carl.bergquist@gmail.com>
Signed-off-by: bergquist <carl.bergquist@gmail.com>
Signed-off-by: bergquist <carl.bergquist@gmail.com>
@bergquist bergquist merged commit 7a6bef8 into main May 3, 2024
12 checks passed
@bergquist bergquist deleted the scopes_tree_node branch May 3, 2024 07:48

type ScopeNodeSpec struct {
//+optional
ParentName *string `json:"parentName"`
Copy link
Contributor

@kylebrandt kylebrandt May 7, 2024

Choose a reason for hiding this comment

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

Does this need to be a pointer? Would empty string suffice for "No parent", or does null and empty have different meanings?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/backend no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants