Add granular control of query service components #3485

jpellizzari · 2023-10-12T20:49:28Z

Allow for granular controls of when Explorer replaces the current table UI/backend.

To test locally, turn off the NATIVE_BUILD flag with unset NATIVE_BUILD, then the Helm chart values changes should work.

Breaking Change release note copy:

⚠️ Breaking Change ⚠️

We have changed the way the explorer feature is enabled. It is now possible to enable or disable explorer on different parts of the application. Previously, there was a global toggle set in the Helm Chart values called enableExplorer. This flag has been replaced by two keys: explorer.enabled and explorer.enabledFor:

explorer:
  enabled: true # global enable/disable flag
  # ...
  enabledFor: # list of components that can be enabled or disabled
    - applications
    - sources
    - gitopssets
    - templates

Adding or removing items from this list will control which parts of the UI utilize the explorer backend.

The current values that are supported for this key:

    - applications
    - sources
    - gitopssets
    - templates

See this section of the docs for more info: https://docs.gitops.weave.works/docs/explorer/getting-started/

opudrovs · 2023-10-16T19:45:52Z

I see the following error when testing this branch with make cluster-dev and NATIVE_BUILD unset:

Is it that yaml or d3 import issue again, just in WGE this time?

weaveworks/weave-gitops#2305
weaveworks/weave-gitops#3672

Will try it on the clean cluster again now.

opudrovs · 2023-10-16T20:15:10Z

The same error. Can check main in an hour.

opudrovs · 2023-10-16T21:48:03Z

Yeah, the UI works fine for me in the main branch, but in the current one I see the same error.

foot · 2023-10-17T08:09:42Z

charts/mccp/values.yaml

@@ -213,16 +213,18 @@ cluster-controller:
        tag: v1.5.2

 #### Explorer
-# Turns on explorer to true
-enableExplorer: false


This change looks good!.. I think quite a few clusters (demoX, maybe some customers) etc will be using this old value.

We could complicate the code w/ a bunch of

{{- if or (.Values.explorer.enabled .Values.explorerEnabled) }}

But also users will notice the explorer has been disabled, check the release notes which will call this out, and turn it back on, no biggie I think? cc @enekofb

There will be a corresponding docs change: https://github.com/weaveworks/weave-gitops/pull/4088/files

If we think backwards compatibility is important here, I can add that.

I think we are fine with having it:

announced as a breaking change in the release notes and

how users could adapt the configuration.

We have it marked as alpha https://docs.gitops.weave.works/docs/explorer/intro/ to given users the right expectations of: yes it improves the situation but is not yet proven to reach a level of maturity to be proven useful for range of scenario.

i definitely think after this change is in we shoudl engage with Steve F. and Darryl to get more feedback though in particular on the explorer-based UIs

jpellizzari · 2023-10-18T16:58:41Z

Yeah, the UI works fine for me in the main branch, but in the current one I see the same error.

@opudrovs Can you try rebasing, then doing a yarn install to ensure that you have the latest deps?

opudrovs · 2023-10-18T17:01:58Z

@jpellizzari yes, just a moment.

enekofb

Tested with

explorer:
  enabled: true
  enabledFor:
    - applications
    - sources
    - gitopssets
    - templates

with these args

I could see Explorer enabled

but i dont see the other ones

looking at

To test locally, turn off the NATIVE_BUILD flag with unset NATIVE_BUILD, then the Helm chart values changes should work.

charts/mccp/templates/clusters-service/deployment.yaml

enekofb · 2023-10-19T15:10:08Z

charts/mccp/templates/clusters-service/deployment.yaml

@@ -53,6 +53,10 @@ spec:
            {{ if .Values.explorer.cleaner.disabled }}
            - --explorer-cleaner-disabled=true
            {{- end }}
+
+            # {{- if .Values.explorer.enabledFor }}
+            - --explorer-enabled-for=applications,sources


what is the background of adding it as flag over adding it to the configmap https://github.com/weaveworks/weave-gitops-enterprise/blob/17d720df8a2c3fa36547537da6f17d6c1934e698/charts/mccp/templates/clusters-service/configmap.yaml#L58

?

This was a leftover debug line that has been fixed. We now iterate through the items in the Values.EnabledFor field. This was also causing the issue you reported with gitopssets not using Explorer.

sorry, i mean passing configuration as runtime flags like this vs just adding them to the config map ?

https://github.com/weaveworks/weave-gitops-enterprise/pull/3485/files#diff-95ac9e88b54348348d8a588312fe429de368aab3f45a8d6f69e2ccd4dfa5f101R61-R62

it feels that having all configuration values in the configmap simplifies config management

charts/mccp/templates/clusters-service/deployment.yaml

enekofb · 2023-10-19T15:22:58Z

charts/mccp/values.yaml

@@ -213,16 +213,18 @@ cluster-controller:
        tag: v1.5.2

 #### Explorer
-# Turns on explorer to true
-enableExplorer: false


I think we are fine with having it:

announced as a breaking change in the release notes and

how users could adapt the configuration.

We have it marked as alpha https://docs.gitops.weave.works/docs/explorer/intro/ to given users the right expectations of: yes it improves the situation but is not yet proven to reach a level of maturity to be proven useful for range of scenario.

i definitely think after this change is in we shoudl engage with Steve F. and Darryl to get more feedback though in particular on the explorer-based UIs

enekofb

tested without native build and

explorer:
  enabled: true
  enabledFor:
    - applications
    - sources
    - gitopssets
    - templates

I could see apps and sources with explorer

but not gitopsssets nor templates

is this expected?

jpellizzari · 2023-10-20T16:24:33Z

is this expected?

@enekofb My leftover debug efforts were causing issues where we didn't loop over the items in the values.yaml. Fixed now!

enekofb

Super! Ive verified worked with all values selected and some selected 🚀

Limitation: my frontend review not of much value (as usual)
Doubts:

asked some question on passing the configuration as command arguments vs configmap
also let please add info on how we are going to communicate the breaking change: for example i recently have one with label added and the notes (add internal monitoring server for metrics and profiling #3500)

charts/mccp/templates/clusters-service/deployment.yaml

jpellizzari · 2023-10-23T22:58:25Z

@enekofb Updated to use configmap, and added a note that can hopefully be copy/pasted into our release notes

@foot See above re breaking change release note.

enekofb

just small comment below

enekofb · 2023-10-24T14:25:02Z

ui/src/components/Applications/index.tsx

@@ -60,7 +59,7 @@ const WGApplicationsDashboard: FC = ({ className }: any) => {
            <OpenedPullRequest />
          </Flex>
          <div className={className}>
-            {useQueryServiceBackend ? (
+            {isQueryServiceEnabled ? (


we might want use isExplorerEnabled as we have other uis https://github.com/weaveworks/weave-gitops-enterprise/pull/3485/files#diff-b9da4e2b2d5014f5b181f3a2c849a4cfbe3f3ac4b537b618c0ecb24e650ba267R44

or the other way around isQueryServiceEnabled ... explorer seems clearer to me though

We want to be able to enable and disable the Explorer and query service backend for different parts of the UI. Before this, it was a global on-off switch handled via feature flags. This commit adds a dedicated enpoint for us to hit to enable or disable components via the helm values file.

Consolidates all of the explorer related configuration options underneath the `enabled` yaml key for consistency.

jpellizzari added the enhancement New feature or request label Oct 16, 2023

jpellizzari force-pushed the 3424-explorer-feature branch 3 times, most recently from f96ad92 to 5b62bf1 Compare October 16, 2023 18:21

jpellizzari requested review from enekofb, opudrovs and foot October 16, 2023 18:39

jpellizzari marked this pull request as ready for review October 16, 2023 18:39

jpellizzari force-pushed the 3424-explorer-feature branch from 1f31d6b to 535cdf0 Compare October 16, 2023 18:54

foot reviewed Oct 17, 2023

View reviewed changes

jpellizzari mentioned this pull request Oct 17, 2023

update explorer docs with granular configuration weaveworks/weave-gitops#4088

Merged

opudrovs force-pushed the 3424-explorer-feature branch from 46a94ed to 5bf1d33 Compare October 18, 2023 17:02

jpellizzari force-pushed the 3424-explorer-feature branch from 5bf1d33 to 17d720d Compare October 18, 2023 17:04

enekofb reviewed Oct 19, 2023

View reviewed changes

enekofb self-requested a review October 19, 2023 15:58

enekofb reviewed Oct 19, 2023

View reviewed changes

jpellizzari force-pushed the 3424-explorer-feature branch from 191a08c to cfdbfc7 Compare October 20, 2023 16:25

enekofb reviewed Oct 23, 2023

View reviewed changes

charts/mccp/templates/clusters-service/deployment.yaml Outdated Show resolved Hide resolved

jpellizzari force-pushed the 3424-explorer-feature branch from fe84c79 to 1bed81e Compare October 23, 2023 22:36

jpellizzari requested review from enekofb and foot October 23, 2023 22:56

enekofb added the breaking-change to flag breaking changes similar to weave gitops oss label Oct 24, 2023

enekofb reviewed Oct 24, 2023

View reviewed changes

enekofb self-requested a review October 24, 2023 14:45

enekofb approved these changes Oct 24, 2023

View reviewed changes

jpellizzari force-pushed the 3424-explorer-feature branch from 1bed81e to 11acba5 Compare October 24, 2023 16:52

jpellizzari added 9 commits October 25, 2023 10:06

Add enabled component logic for applications and sources

2ce1592

Enable explorer for gitopssets and templates

41311d4

Simplify option append

e040b18

Remove separate enableExplorer flag from helm chart values

9de4ee0

Consolidates all of the explorer related configuration options underneath the `enabled` yaml key for consistency.

Fix protobuf mispelling

3b89697

Add proto comment

09f0bf9

Add template logic for explorer-enabled-for flag

b7040d9

Move explorer-enabled-for to configmap

f08c34f

jpellizzari force-pushed the 3424-explorer-feature branch from 11acba5 to f08c34f Compare October 25, 2023 17:10

Make explorer variable name consistent

2df992d

jpellizzari merged commit 3c8f577 into main Oct 25, 2023

jpellizzari deleted the 3424-explorer-feature branch October 25, 2023 17:26

foot mentioned this pull request Nov 8, 2023

Adds AutomatedClusterDiscovery to the explorer #3472

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add granular control of query service components #3485

Add granular control of query service components #3485

jpellizzari commented Oct 12, 2023 •

edited

Loading

opudrovs commented Oct 16, 2023 •

edited

Loading

opudrovs commented Oct 16, 2023 •

edited

Loading

opudrovs commented Oct 16, 2023 •

edited

Loading

foot Oct 17, 2023 •

edited

Loading

jpellizzari Oct 17, 2023

enekofb Oct 19, 2023

jpellizzari commented Oct 18, 2023

opudrovs commented Oct 18, 2023

enekofb left a comment •

edited

Loading

enekofb Oct 19, 2023

jpellizzari Oct 20, 2023

enekofb Oct 23, 2023

enekofb Oct 19, 2023

enekofb left a comment

jpellizzari commented Oct 20, 2023

enekofb left a comment

jpellizzari commented Oct 23, 2023

enekofb left a comment •

edited

Loading

enekofb Oct 24, 2023

Add granular control of query service components #3485

Add granular control of query service components #3485

Conversation

jpellizzari commented Oct 12, 2023 • edited Loading

opudrovs commented Oct 16, 2023 • edited Loading

opudrovs commented Oct 16, 2023 • edited Loading

opudrovs commented Oct 16, 2023 • edited Loading

foot Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

jpellizzari Oct 17, 2023

Choose a reason for hiding this comment

enekofb Oct 19, 2023

Choose a reason for hiding this comment

jpellizzari commented Oct 18, 2023

opudrovs commented Oct 18, 2023

enekofb left a comment • edited Loading

Choose a reason for hiding this comment

enekofb Oct 19, 2023

Choose a reason for hiding this comment

jpellizzari Oct 20, 2023

Choose a reason for hiding this comment

enekofb Oct 23, 2023

Choose a reason for hiding this comment

enekofb Oct 19, 2023

Choose a reason for hiding this comment

enekofb left a comment

Choose a reason for hiding this comment

jpellizzari commented Oct 20, 2023

enekofb left a comment

Choose a reason for hiding this comment

jpellizzari commented Oct 23, 2023

enekofb left a comment • edited Loading

Choose a reason for hiding this comment

enekofb Oct 24, 2023

Choose a reason for hiding this comment

jpellizzari commented Oct 12, 2023 •

edited

Loading

opudrovs commented Oct 16, 2023 •

edited

Loading

opudrovs commented Oct 16, 2023 •

edited

Loading

opudrovs commented Oct 16, 2023 •

edited

Loading

foot Oct 17, 2023 •

edited

Loading

enekofb left a comment •

edited

Loading

enekofb left a comment •

edited

Loading