weaveworks / weave-gitops

Weave GitOps provides insights into your application deployments, and makes continuous delivery with GitOps easier to adopt and scale across your teams.
https://docs.gitops.weave.works/
Apache License 2.0
905 stars 151 forks source link

allow extending flux runtime ui beyond flux to create gitops runtime #4162

Closed enekofb closed 8 months ago

enekofb commented 9 months ago

Closes of https://github.com/weaveworks/weave-gitops/issues/4174

What changed?

Why was this change made?

How was this change implemented?

How did you validate the change?

Documentation Changes

enekofb commented 8 months ago

next is to refactor logic to:

opudrovs commented 8 months ago

Some tests are failing.

Will test the current PR after I am done with testing my enterprise PR (building it now).

enekofb commented 8 months ago

Some tests are failing.

Will test the current PR after I am done with testing my enterprise PR (building it now).

@opudrovs sorry i assigned request too early, ignore it so far ... just reviewing the PR and need to add questions to ask.

opudrovs commented 8 months ago

@enekofb ah got it, np. 👌

opudrovs commented 8 months ago

❔ Will any of new components/hooks/functions be used in enterprise directly? I mean, should anything be added to exports in ui/index.ts?

opudrovs commented 8 months ago

❔ It's not an issue, just a minor question, related to usability (and I am not sure who will use this new UI, so don't know if it's of any importance). Since the routes of both views are slightly different:

/flux_runtime/flux
/runtime/flux

if the user toggles the feature flag and just refreshes the page (while browser stays at the same route), there will be a page not found error displayed:

Screenshot 2023-12-22 at 03 55 23

Is it possible that someone will share a link to a runtime page (in a documentation, for example) and then the flag will be toggled and the users will bump into page not working? Would a redirect or keeping both routes functional help? If it does not matter (or like in "if anyone starts screaming about it, we'll figure it out then" 😅 ), never mind.

enekofb commented 8 months ago

❔ Will any of new components/hooks/functions be used in enterprise directly? I mean, should anything be added to exports in ui/index.ts?

Interesting question that clashes with my existing FE knowledge: yes, we would like to use the feature as is in EE. it is the second story on this epic https://github.com/weaveworks/weave-gitops-enterprise/issues/3725

What changes needs to do in this PR to just use it in EE?

enekofb commented 8 months ago

❔ It's not an issue, just a minor question, related to usability (and I am not sure who will use this new UI, so don't know if it's of any importance). Since the routes of both views are slightly different:

/flux_runtime/flux
/runtime/flux

if the user toggles the feature flag and just refreshes the page (while browser stays at the same route), there will be a page not found error displayed:

Screenshot 2023-12-22 at 03 55 23

Is it possible that someone will share a link to a runtime page (in a documentation, for example) and then the flag will be toggled and the users will bump into page not working? Would a redirect or keeping both routes functional help? If it does not matter (or like in "if anyone starts screaming about it, we'll figure it out then" 😅 ), never mind.

Yes, it is a bit annoying but think of a limited impact ( i think only once) as you face that situation in a scenario where there an existing WEGO installed. Given that its a configuration issue, it will be noticed by the platfrom eng or the dev doing the upgrade or configuration. The rest of the wego users, given there is a new release of the app would need to relogin and use whatever route is there via navigation so hopefully not that impacted.

Having both routes /flux_runtime and /runtime feels semantically incorrect in terms of exectations for users going to /flux_runtime and potentially seeing other non-flux controllers

My 2cts is that we could deliver as is and as proven value, we deprecate flux_runtime as a sub-set of runtime ... if not value for runtime ... then we just dont ship it for OSS (only for EE )

How do you feel about that @opudrovs ?

enekofb commented 8 months ago

@opudrovs re-requested review after addressing comments. PTAL 🙏

opudrovs commented 8 months ago

Having both routes /flux_runtime and /runtime feels semantically incorrect in terms of exectations for users going to /flux_runtime and potentially seeing other non-flux controllers

My 2cts is that we could deliver as is and as proven value, we deprecate flux_runtime as a sub-set of runtime ... if not value for runtime ... then we just dont ship it for OSS (only for EE )

@enekofb I see, thanks, fine with me.

opudrovs commented 8 months ago

@enekofb

Interesting question that clashes with my existing FE knowledge: yes, we would like to use the feature as is in EE. it is the second story on this epic https://github.com/weaveworks/weave-gitops-enterprise/issues/3725

What changes needs to do in this PR to just use it in EE?

It's more related to our specific product setup and to how we build the OSS package (@weaveworks/weave-gitops) which is imported to enterprise not frontend in general, you frontend knowledge is fine. 😸

In order to be able to import an identifier in enterprise from OSS (I mean directly, if it's not a part of an already imported component), you need to export it from OSS in ui/index.ts. Just import them to index.ts and add them to the long list of exports: https://github.com/weaveworks/weave-gitops/blob/78d5f6dc24cbc647800bfeed3691c80c517d2edf/ui/index.ts#L143

I am not sure what should be added to the exports, it depends on what will be imported when the new components/function are used in enterprise. So, feel free to add the exports now or in the next issue.

enekofb commented 8 months ago

@enekofb

Interesting question that clashes with my existing FE knowledge: yes, we would like to use the feature as is in EE. it is the second story on this epic weaveworks/weave-gitops-enterprise#3725

What changes needs to do in this PR to just use it in EE?

It's more related to our specific product setup and to how we build the OSS package (@weaveworks/weave-gitops) which is imported to enterprise not frontend in general, you frontend knowledge is fine. 😸

In order to be able to import an identifier in enterprise from OSS (I mean directly, if it's not a part of an already imported component), you need to export it from OSS in ui/index.ts. Just import them to index.ts and add them to the long list of exports:

https://github.com/weaveworks/weave-gitops/blob/78d5f6dc24cbc647800bfeed3691c80c517d2edf/ui/index.ts#L143

I am not sure what should be added to the exports, it depends on what will be imported when the new components/function are used in enterprise. So, feel free to add the exports now or in the next issue.

done! https://github.com/weaveworks/weave-gitops/pull/4162/commits/93d08fed0fe5403eef9f92666993e05c2a1c4ddf