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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(dataplanes): integrates Gateways in Data Plane Proxy list view #1728

Merged
merged 2 commits into from
Nov 9, 2023
Merged

feat(dataplanes): integrates Gateways in Data Plane Proxy list view #1728

merged 2 commits into from
Nov 9, 2023

Conversation

kleinfreund
Copy link
Contributor

@kleinfreund kleinfreund commented Nov 6, 2023

Changes

chore(mocks): update mocks to handle gateway types properly

Updates the dataplane mocks to handle gateway types more properly. The list views now generate names indicating which type of gateway is used so we can properly respond to this in other mocks (e.g. for Dataplane policies, we should only show the gateway policy list for builtin gateways).

feat(dataplanes): integrates Gateways in Data Plane Proxy list view

Integrates the Gateway list view in the Data Plane Proxy list view. Adds a type column and the appropriate type filter to the DPP list view and the service detail view’s DPP list.

Removes the protocol and zone columns.

Resolves #1717.

Resolves #1407.

Signed-off-by: Philipp Rudloff [email protected]

Notes

  • For the type column, how should we display the values:
    • For the two gateway types, should we keep them uppercase (i.e. "BUILTIN" and "DELEGATED")?
    • For regular DPPs, should we show "STANDARD" or omit the value or something else?

@kleinfreund kleinfreund self-assigned this Nov 6, 2023
Copy link

netlify bot commented Nov 6, 2023

Deploy Preview for kuma-gui ready!

Name Link
🔨 Latest commit 95f08c5
🔍 Latest deploy log https://app.netlify.com/sites/kuma-gui/deploys/654cdc0a2a90d30008e93952
😎 Deploy Preview https://deploy-preview-1728--kuma-gui.netlify.app/gui
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@kleinfreund kleinfreund marked this pull request as ready for review November 7, 2023 15:33
@kleinfreund kleinfreund requested a review from a team as a code owner November 7, 2023 15:33
@kleinfreund kleinfreund requested review from johncowen and removed request for a team November 7, 2023 15:33
@johncowen
Copy link
Contributor

Just taking a quick look at this now, but:

For the two gateway types, should we keep them uppercase (i.e. "BUILTIN" and "DELEGATED")?
For regular DPPs, should we show "STANDARD" or omit the value or something else?

How about "Built-in Gateway", "Delegated Gateway" and "Standard Proxy"?

Copy link
Contributor

@johncowen johncowen left a comment

Choose a reason for hiding this comment

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

I had a quick look at this last thing and had some comments, figured I'd submit those before coming back here tomorrow to finish up. Looking great tho!

src/app/data-planes/components/DataPlaneList.vue Outdated Show resolved Hide resolved
src/app/data-planes/components/DataPlaneList.vue Outdated Show resolved Hide resolved
src/test-support/mocks/src/dataplanes/_overview.ts Outdated Show resolved Hide resolved
@kleinfreund
Copy link
Contributor Author

How about "Built-in Gateway", "Delegated Gateway" and "Standard Proxy"?

I moved the values of the filter and those in the list into the message files so we only need to adjust them there now based on what we decide.

I used those terms for now. Will bring it up again when this is close to done.

@kleinfreund
Copy link
Contributor Author

kleinfreund commented Nov 8, 2023

I’ll be adding some tests here that make sure that the DPP detail view’s Policies tab always uses the right "mode" depending on Dataplane type. I noticed I could easily get tests to pass while not handling this correctly.

Update: This is done now (see features/mesh/dataplanes/DataplanePolicies.feature).

@kleinfreund kleinfreund requested a review from johncowen November 8, 2023 09:46
@johncowen
Copy link
Contributor

I’ll be adding some tests here that make sure that the DPP detail view’s Policies tab always uses the right "mode" depending on Dataplane type. I noticed I could easily get tests to pass while not handling this correctly.

Awesome, good spot! let me take another look through this.

@kleinfreund
Copy link
Contributor Author

I cleaned up the mocks for dataplanes some more.

I noticed we weren’t correctly reading a tag-based filter (we accessed tags instead of tag). I made it so that now the service detail view’s DPP list actually only shows DPPs with the expected type plus made it so that its service tags will also match the service.

That’s the last of what I wanted to address in this PR.

Copy link
Contributor

@johncowen johncowen left a comment

Choose a reason for hiding this comment

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

I think this is almost good to go

I spotted this:

Screenshot 2023-11-08 at 11 11 11

The filter bar being too small ^

Other than that I think I had one question about tags: {} in the inline comments, the rest I think is just talking out loud. Let me know and I think we are good to go with this 👍

features/mesh/dataplanes/Index.feature Outdated Show resolved Hide resolved
features/mesh/dataplanes/Index.feature Show resolved Hide resolved
@kleinfreund kleinfreund requested a review from johncowen November 9, 2023 12:16
Philipp Rudloff added 2 commits November 9, 2023 14:17
Updates the dataplane mocks to handle gateway types more properly. The list views now generate names indicating which type of gateway is used so we can properly respond to this in other mocks (e.g. for Dataplane policies, we should only show the gateway policy list for builtin gateways).

Updates all dataplane mocks to be more self-consistent.

Signed-off-by: Philipp Rudloff <[email protected]>
Integrates the Gateway list view in the Data Plane Proxy list view. Adds a type column and the appropriate type filter to the DPP list view and the service detail view’s DPP list.

Removes the protocol and zone columns.

Signed-off-by: Philipp Rudloff <[email protected]>
Copy link
Contributor

@johncowen johncowen left a comment

Choose a reason for hiding this comment

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

The filter bar being too small ^

Looks to me like the filter bar thing is fixed here now?

In that case this LGTM!

@kleinfreund
Copy link
Contributor Author

Looks to me like the filter bar thing is fixed here now?

Yep.

@kleinfreund kleinfreund merged commit dbdeec7 into kumahq:master Nov 9, 2023
15 checks passed
@kleinfreund kleinfreund deleted the feat/combines-gateways-and-dpp-list-views branch November 9, 2023 13:26
@kleinfreund kleinfreund removed their assignment Nov 9, 2023
johncowen added a commit that referenced this pull request Nov 14, 2023
…ll (#1756)

The long story:

During #1728 a DataSource source was removed (https://github.com/kumahq/kuma-gui/pull/1728/files#diff-8c51094d188779df11ec5a418503cf5a77c088bb12581335955952e7deef4c3dL50-L65), and replaced by https://github.com/kumahq/kuma-gui/pull/1728/files#diff-8c51094d188779df11ec5a418503cf5a77c088bb12581335955952e7deef4c3dR100-R122

Then, as well as this usage:

https://github.com/kumahq/kuma-gui/pull/1728/files#diff-9d4c2f48081cf9b206eb208a8f15ad06ede0411df7c550dbd938e1926d89b2d9L21

... there is another usage of this DataSource for onboarding which, if you only have 1 mesh called `default` would use this DataSource to figure out if you had added any dataplanes yet, if not we would show you the onboarding notification.

Since the source was removed, the DataSource would error with a DataSource not found error.

---

Firstly, I added the source back and then added a test to cover this eventuality, as well as another for the negative.

As we had discussed #1746, I also made some small changes here to:

1. Restrict the OnboardingNotification to the homepage by checking for `route.name === 'home'`
2. Use the information already available about dataplanes in the `/mesh-insights` API endpoint, meaning we no longer have to make the additional HTTP request to inspect the amount of dataplanes. This then meant I could re-remove the source whose removal caused the error in the first place.
3. Add a further test to cover the "only show this on the homepage" case
4. As mentioned in #1746 (comment) I also made sure that the `?size=`s used for the homepage and the onboarding notification where them same, and could therefore re-use the same request.
5. Note I am still using the `/mesh-insights` API request. I believe it gives us the same information we would get from `/global-insights` plus it also means we can inspect the name of the only mesh. This means if the only mesh is called `default`, then its a possibly a fresh install and we may show the onboarding, if its called `another-mesh` then it's not a fresh install and we shouldn't ever show the onboarding.

Potential further improvement work that is not necessary in this PR:

1. I left a TODO in the mocks to properly use `KUMA_DATAPLANE_COUNT`, which will then need 'partioning' to give us a spread of types from the total requested.
2. Now that onboarding is only on the homepage we should extract it from ApplicationShell, and embed it into ControlPlaneStatus (the 'status' being "is this control plane fresh or not"). Importantly we should try to do this "as a micro service" so that when our onboarding isn't installed at all then we don't include any code whatsover to do with onboarding.

I did not add the help menu item for onboarding here and we should do that as a separate PR aside from the bugfix PR.




Signed-off-by: John Cowen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dataplane and gateway simplification Data Plane Proxies show only first service/zone from tags
2 participants