feat: add a dynamic, relation-driven sidebar #118
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Adds: * KubeflowDashboardSidebarRequirer * KubeflowDashboardSidebarProvider * SidebarItem * associated tests
Used as a helper in unit tests
This change delays the computation of the k8s context until it is needed, instead of in __init__. It does not change the outward function of the charm. This change was implemented because in future, the sidebar links will be obtained from a relation and this should not be done in __init__.
previously, the Provider lib had a bug that would return only the last relation's data, not the entire set of data. This fixes that bug.
Includes unit tests
This adds the feature to get_sidebar_items to, when executed during a relation-broken event for this relation, omit the relation data for the breaking app (the app leaving the relation). For context, see https://chat.charmhub.io/charmhub/pl/ Possibly controversially, this feature uses the Juju environment variables to determine what event type is being executed. This is to avoid having to pass the `BoundEvent` object to every `get_sidebar_items()` call, and to keep this hack entirely in the relation library so the charm does not need to know about it.
Previously, the links portion of the kubeflow dashboard configmap was incorrectly rendering only the menulinks, not all links.
jnsgruk
reviewed
Jun 27, 2023
i-chvets
reviewed
Jun 27, 2023
i-chvets
reviewed
Jun 27, 2023
i-chvets
reviewed
Jun 27, 2023
Removes tests for previously defined static links.
i-chvets
reviewed
Jun 27, 2023
i-chvets
reviewed
Jun 27, 2023
i-chvets
reviewed
Jun 27, 2023
|
Left some comments.LGTM otherwise. |
1 task
|
Not sure what is wrong with the selenium integration tests. They might be failing because they're stuck on the profile creation page, but I don't know why this PR would have caused that. Need to look into that more |
When refactoring the template, the links were changed by mistake. This reverts those changes.
Previously, the lib was defined as v1, but the original v0 was never published so we have to re-use v0.
ca-scribner
force-pushed
the
kf-3664-add-dynamic-sidebar-lib
branch
from
ecacdd8
to
e880538
Compare
ca-scribner
added a commit
that referenced
this pull request
Jul 12, 2023
Changes include: * adding KubeflowDashboardSidebarRequirer * adding KubeflowDashboardSidebarProvider * adding SidebarItem * adding associated tests, including a Requires side tester for the new relation * adding the sidebar relation and using the KubeflowDashboardSidebarProvider to manage it * refactoring computation of k8s context until it is needed, instead of in `__init__`, to make testing easier
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR refactors how kubeflow dashboard's sidebar is implemented. Previously, the sidebar was defined statically in a configmap template. This PR removes the static definition, replacing it with links that are requested by other applications through a new
sidebarrelation. Full details are here.Added here are:
lib/charms/kubeflow_dashboard/v1/kubeflow_dashboard_sidebar.py: Requirer and Provider side handler for thesidebarrelationtests/integration/sidebar_requirer_tester_charm: a mock charm that uses the Requirer side of the library. This is used in integration tests and can be used in manual testingNotes for reviewer:
tests/integration/sidebar_requirer_tester_charm(which is a simple implementation of theRequirerside of the relation) and relate it to the kubeflow-dashboard charmOut of scope (will be covered in separate PRs):
Todo at merge
Closes #8