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

Extend explorer - Templates #3358

Merged

ranatrk merged 10 commits into main from extend-explorer-templates

Sep 26, 2023

Contributor

ranatrk commented Sep 14, 2023 •

edited

Loading

Closes #2957

What changed?
Add capiTemplates and gitopsTemplates to explorer filters

Why was this change made?
Extending the explorer to be compatible to filter based on the kind CapiTemplates and GitOpsTemplates

How was this change implemented?
Add both templates to compatible objects used in the explorer filter
Returning no status when getting flux object to show no status in the ui

How did you validate the change?

Explain how a reviewer can verify the change themselves
manual test: Add a template(capi or gitops), then through the explorer view tab, enable the filter to only show that template type

Release notes
Add templates,Capi and GitOps, to explorer filters

Documentation Changes
N/A

Other follow ups

ranatrk added the enhancement label

ranatrk force-pushed the extend-explorer-templates branch 2 times, most recently from 0c3cc5b to a302b9a Compare

September 17, 2023 10:54

ranatrk marked this pull request as ready for review

September 17, 2023 11:26

ranatrk requested review from jpellizzari and enekofb

September 17, 2023 11:26

Contributor Author

ranatrk commented Sep 17, 2023

the current state in this PR is that templates will direct to the main templates page, where each template doesn't have a details page. There is also another option that I can remove the redirect so the yaml of the templates show in the explorer details rather than directing to the /templates endpoint.
@enekofb wdyt?

enekofb requested a review from a team

September 18, 2023 07:05

Contributor

enekofb commented Sep 18, 2023 •

edited

Loading

There is also another option that I can remove the redirect so the yaml of the templates show in the explorer details rather than directing to the /templates endpoint.

As a user, once you have found something in explorer you click to be able to do somethingelse with the found resource, i could think that in the context of templates is going to the templates page over the unstructured yaml (it does not allow the user to do another action than view raw data).

A third option (that you could consider) is to redirect to Templates view with focus in the selected template by using a URL that searches by the name of the template. Something like this

Given explorer with a template aws-eks-dev
When i select the template
Then templates view is opened with the template as the search result
So the user can view more specific details for he resource of interest and act on it (use the template)

enekofb reviewed

View reviewed changes

Contributor

enekofb left a comment

Reviewed the code with some suggestions. Not yet tested as some of the suggestions could change how the feature behaves.

pkg/query/configuration/objectkind.go Outdated

+              		},
+              		AddToSchemeFunc: gapiv1.AddToScheme,
+              		StatusFunc: func(obj client.Object) ObjectStatus {
+              			e, ok := obj.(*corev1.Event)

Contributor

enekofb Sep 18, 2023

given obj represents this interface which is a generic kube object, and you will use this function for templates, then the casting to Event on this line would always fail.

If you don't expect a status different to NoStatus, you could just return it directly without any other logic for this function.

pkg/query/configuration/objectkind.go Outdated

+              			return Failed
+              		},
+              		MessageFunc: func(obj client.Object) string {
+              			e, ok := obj.(*corev1.Event)

Contributor

enekofb Sep 18, 2023

Same comment as mentioned for StatusFunc. Here you could consider whether you want to use this field for giving another type of information to the user in a simplified manner to help them select: I could think that an option could be to return the description if that suits you.

Contributor Author

ranatrk Sep 18, 2023

looking at the templates object, the description is probably the only item that could be used. But would it be confusing to the user to have some messages indicating the status message as actions while the templates have a description
action format example : Release reconciliation succeeded
description format example: Add a Helm Repository to all clusters

pkg/query/configuration/objectkind.go

+              			return &capiv1.CAPITemplate{}
+              		},
+              		AddToSchemeFunc: capiv1.AddToScheme,
+              		StatusFunc: func(obj client.Object) ObjectStatus {

Contributor

enekofb Sep 18, 2023

same comment as for GitopsTemplate

pkg/query/configuration/objectkind.go Outdated

+              			return Failed
+              		},
+              		MessageFunc: func(obj client.Object) string {
+              			e, ok := obj.(*corev1.Event)

Contributor

enekofb Sep 18, 2023

same comment as for GitopsTemplate

pkg/query/configuration/objectkind.go Outdated

               	if err != nil {
               		return Failed
               	}
+              	if fo == nil {

Contributor

enekofb Sep 18, 2023

do you still need this?

Contributor Author

ranatrk Sep 18, 2023

no, it's replaced with the custom status and message functions

pkg/query/server/suite_test.go

@@ @@ -99,6 +101,16 @@ func TestMain(m *testing.M) { @@
               	if err != nil {
               		log.Fatalf("add GitopsSet to schema failed: %s", err)
               	}
+              	err = gapiv1.AddToScheme(scheme.Scheme)

Contributor

enekofb Sep 18, 2023

you might want also to add a test case (as you did for gitopssets) for template resources

Contributor Author

ranatrk commented Sep 18, 2023

A third option (that you could consider) is to redirect to Templates view with focus in the selected template by using a URL that searches by the name of the template. Something like this

Given explorer with a template aws-eks-dev When i select the template Then templates view is opened with the template as the search result So the user can view more specific details for he resource of interest and act on it (use the template)

that's a nice idea, I'll look into the templates FE code to see it's feasibility

jpellizzari approved these changes

View reviewed changes

jpellizzari left a comment

Code LGTM! Let us know if we need to update our extending-explorer.md README with any new information.

ranatrk force-pushed the extend-explorer-templates branch from 45615ed to 6823e84 Compare

September 19, 2023 12:40

Contributor Author

ranatrk commented Sep 19, 2023

current state:

Custom adapters are created for the template objects to be considered as fluxObjects with GetConditions function.
The objects being cast in the custom MessageFunc are the adapters and not GitOpsTemplate and CAPITemplate objects

Collaborator

foot commented Sep 20, 2023

Thanks @ranatrk! We were looking at the code and wondering if we could remove the explicit call to ToFluxObject inside of GetStatus and GetMessage?

weave-gitops-enterprise/pkg/query/internal/models/object.go

Lines 113 to 129 in 9df49da

    
           func (n defaultNormalizedObject) GetStatus() (configuration.ObjectStatus, error) { 
        
           	o, err := configuration.ToFluxObject(n.Object) 
        
           	if err != nil { 
        
           		return "", err 
        
           	} 
        
           	return n.config.StatusFunc(o), nil 
        
           } 
        
           func (n defaultNormalizedObject) GetMessage() (string, error) { 
        
           	o, err := configuration.ToFluxObject(n.Object) 
        
           	if err != nil { 
        
           		return "", err 
        
           	} 
        
           	return n.config.MessageFunc(o), nil 
        
           }

This would mean ToFluxObject would only be called in the defaultFluxObjectStatusFunc / defaultFluxObjectMessageFunc and wouldn't have to worry about handling types other than flux ones.

weave-gitops-enterprise/pkg/query/configuration/objectkind.go

Lines 306 to 307 in 9df49da

    
           func defaultFluxObjectStatusFunc(obj client.Object) ObjectStatus { 
        
           	fo, err := ToFluxObject(obj)

cc @enekofb does that make sense? There might be extra context.

Contributor

enekofb commented Sep 21, 2023 •

edited

Loading

Thanks @ranatrk! We were looking at the code and wondering if we could remove the explicit call to ToFluxObject inside of GetStatus and GetMessage?

weave-gitops-enterprise/pkg/query/internal/models/object.go

Lines 113 to 129 in 9df49da

func (n defaultNormalizedObject) GetStatus() (configuration.ObjectStatus, error) {

o, err := configuration.ToFluxObject(n.Object)

if err != nil {

return "", err

}

return n.config.StatusFunc(o), nil

}

func (n defaultNormalizedObject) GetMessage() (string, error) {

o, err := configuration.ToFluxObject(n.Object)

if err != nil {

return "", err

}

return n.config.MessageFunc(o), nil

}

This would mean ToFluxObject would only be called in the defaultFluxObjectStatusFunc / defaultFluxObjectMessageFunc and wouldn't have to worry about handling types other than flux ones.

weave-gitops-enterprise/pkg/query/configuration/objectkind.go

Lines 306 to 307 in 9df49da

func defaultFluxObjectStatusFunc(obj client.Object) ObjectStatus {

fo, err := ToFluxObject(obj)

cc @enekofb does that make sense? There might be extra context.

@foot definitely, it needs decoupling ...

raising a PR, should be soon

Contributor

enekofb commented Sep 21, 2023

raised PR for the toFluxObject mandate

ranatrk force-pushed the extend-explorer-templates branch from f30c57a to d73137d Compare

September 24, 2023 13:30

Contributor

enekofb commented Sep 25, 2023

PR merged, please rebase

ranatrk force-pushed the extend-explorer-templates branch from d73137d to e029238 Compare

September 25, 2023 10:57

Contributor Author

ranatrk commented Sep 25, 2023

thanks @enekofb for the updates in the toFluxObject. I rebased and did a minor change in the url for the redirection to templates page

ranatrk requested review from enekofb and foot

September 25, 2023 11:35

enekofb approved these changes

View reviewed changes

Contributor

enekofb left a comment

Great job!

Tested that I could see templates and navigate to them correctly:

Suggest to follow up adding it to the user documentation (rather small change)

ranatrk added 6 commits

September 26, 2023 13:11


          Add gitops and capi templates to explorer

5374c1c

WIP testing


          Add capi and gitops template crds to query server testdata

d19b015

Add templates to ToFluxObject but ignore their status and return NoStatus for them


          Replace default functions for status and message to custom ones for t…

20cdef6

…he template kind objects as they dont have any status


          return NoStatus directly with no checks for the template kind status …

5b0c7df

…func and empty string for the message func


          Add tests for supporting capi and gitops templates in query

067ebf5


          fix redirect to templates from explorer to include name and namespace…

243346a

… only

ranatrk added 4 commits

September 26, 2023 13:11


          Add adapters for capi and gitops templates

ea1204d

WIP return description instead of empty message from templates


          Fix object types in custom message func to custom template adapters

2048c92


          remove capi and gitops templates adapter used to mask template object…

68e483f

…s to fluxObjects


          Update query params of templates redirect url in explorer table

fbccc98

ranatrk force-pushed the extend-explorer-templates branch from f1c8a1f to fbccc98 Compare

September 26, 2023 10:11

ranatrk merged commit 9f7c41e into main

10 checks passed

ranatrk deleted the extend-explorer-templates branch

September 26, 2023 10:57

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels