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

Groups admin panel #270

Merged

Conversation

lucferbux
Copy link
Contributor

@lucferbux lucferbux commented Jul 7, 2022

Groups admin panel

Description

Added a new settings section to manage groups:

  • Ability to control admin groups in the dashboard.
  • Ability to control user groups in the dashbaord.
  • Automatically detects a group selected is removed and notify the user.
  • Adds the ability to support multi group selection.
  • Grant by default permission for accessing settings panel to all the cluster admins

Kfdef

kind: KfDef
apiVersion: kfdef.apps.kubeflow.org/v1
metadata:
  name: opendatahub
  namespace: opendatahub
spec:
  applications:
    - kustomizeConfig:
        repoRef:
          name: manifests
          path: odh-common
      name: odh-common
    - kustomizeConfig:
        parameters:
          - name: s3_endpoint_url
            value: s3.odh.com
        repoRef:
          name: manifests
          path: jupyterhub/jupyterhub
      name: jupyterhub
    - kustomizeConfig:
        overlays:
          - additional
        repoRef:
          name: manifests
          path: jupyterhub/notebook-images
      name: notebook-images
    - kustomizeConfig:
        overlays:
          - authentication
        repoRef:
          name: manifests
          path: odh-dashboard
      name: odh-dashboard
  repos:
    - name: kf-manifests
      uri: >-
        https://github.com/opendatahub-io/manifests/tarball/v1.4.0-rc.2-openshift
    - name: manifests
      uri: 'https://github.com/lucferbux/odh-manifests/tarball/groups-panel'

How Has This Been Tested?

test1

  1. Upload this KfDef.
  2. Disable the operator to be able to modify the configmaps without being reconciled.
  3. Check the instructions in the docs about how the admin dashboard is enabled.
  4. Start adding and deleting groups.
  5. When having a group selected, if it's deleted in the cluster, it will trigger a new notification.

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Github Issue: Enable ODH Dashboard Admin & User groups #192
  • KfDef added.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work.

@lucferbux lucferbux marked this pull request as ready for review July 13, 2022 09:43
@lucferbux lucferbux force-pushed the groups_admin_panel branch 3 times, most recently from f9f63b8 to 8c17cf4 Compare July 13, 2022 12:14
@lucferbux lucferbux self-assigned this Jul 13, 2022
@lucferbux lucferbux added the kind/enhancement New functionality request (existing augments or new additions) label Jul 13, 2022
@lucferbux lucferbux force-pushed the groups_admin_panel branch 2 times, most recently from 55b6a3b to 07a2480 Compare July 13, 2022 13:26
@lucferbux lucferbux force-pushed the groups_admin_panel branch 5 times, most recently from bb2234b to f6e49ae Compare July 15, 2022 09:15
Copy link
Contributor

@maroroman maroroman left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@lucferbux lucferbux changed the title Groups admin panel [WIP: Not related to KFNBC] Groups admin panel Jul 21, 2022
@bdattoma
Copy link

bdattoma commented Jul 22, 2022

as per offline discussion, we should change the following behavior: Adds the ability to access settings panel to cluster admin and namespaced admins if the last group selected is removed by mistake. to Grant by default permission for accessing settings panel to all the cluster admins

Copy link

@bdattoma bdattoma left a comment

Choose a reason for hiding this comment

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

as per offline discussion, we should change the following behavior: Adds the ability to access settings panel to cluster admin and namespaced admins if the last group selected is removed by mistake. to Grant by default permission for accessing settings panel to all the cluster admins

@lucferbux
Copy link
Contributor Author

as per offline discussion, we should change the following behavior: Adds the ability to access settings panel to cluster admin and namespaced admins if the last group selected is removed by mistake. to Grant by default permission for accessing settings panel to all the cluster admins

Changed!

@lucferbux lucferbux changed the title [WIP: Not related to KFNBC] Groups admin panel [Not related to KFNBC] Groups admin panel Jul 27, 2022
@lucferbux lucferbux force-pushed the groups_admin_panel branch 4 times, most recently from c012cc6 to f321195 Compare July 28, 2022 10:07
Copy link
Contributor

@dlabaj dlabaj left a comment

Choose a reason for hiding this comment

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

A few comments nothing that i would hold the PR up for we could fix them later.

@kywalker-rh
Copy link

We should change the design of this to use a drop down and chip groups similar to what is shown here. https://www.patternfly.org/v4/components/toolbar#with-custom-chip-group-content This way we won't have such a large drop down with items showing up. @kywalker-rh do you agree? This can be done in a future release. We could use a simple inline chip group below the drop down. https://www.patternfly.org/v4/components/chip-group

@dlabaj Agreed, the long dropdown looks awkward. FYI @vconzola

@vconzola
Copy link

@kywalker-rh @dlabaj I don't agree with using chip groups for something like this. By "large dropdown", do you mean that the width spans the entire page? If that's what's bothering you, can we just use the isWidthLimited property on the form?

Copy link
Member

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

This looks functionally sound. I'm going to approve and merge this.

My PR (#415) will need a rebase to rework the user related backend logic to what Lucas has done. So I need to merge this now to do that effort.

I'll be logging a follow up ticket to clean up the UI (and a couple of the backend concerns that are still around -- more style of code and maintainability of the React code). The effort should be non-user facing (ideally) and entirely so we can live with this code and have easier refactors in the future.

Thanks for all your work on this @lucferbux -- Again, very sorry it took so long to get this PR in. I was unaware of its importance to KFNBC release until yesterday.

/lgtm
/unhold

@openshift-ci openshift-ci bot added lgtm and removed do-not-merge/hold This PR is hold for some reason labels Aug 25, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 25, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, dlabaj, maroroman

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [andrewballantyne,dlabaj]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@andrewballantyne
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Aug 25, 2022
@andrewballantyne
Copy link
Member

@kywalker-rh @dlabaj I don't agree with using chip groups for something like this. By "large dropdown", do you mean that the width spans the entire page? If that's what's bothering you, can we just use the isWidthLimited property on the form?

@vconzola @dlabaj @kywalker-rh I've added this to the UX meeting later today -- let us talk through it there.

@openshift-merge-robot openshift-merge-robot merged commit c071335 into opendatahub-io:main Aug 25, 2022
LaVLaS pushed a commit to LaVLaS/odh-dashboard that referenced this pull request Aug 26, 2022
LaVLaS referenced this pull request in red-hat-data-services/odh-dashboard Aug 31, 2022
strangiato pushed a commit to strangiato/odh-dashboard that referenced this pull request Oct 18, 2023
jstourac added a commit to jstourac/ods-ci that referenced this pull request Dec 11, 2023
Verify Automatically Detects a Group Selected Is Removed and Notify the User

The error message is now changed so it matches reality brought in by [1]

[1] opendatahub-io/odh-dashboard#270
jgarciao pushed a commit to red-hat-data-services/ods-ci that referenced this pull request Dec 12, 2023
…#1074)

Verify Automatically Detects a Group Selected Is Removed and Notify the User

The error message is now changed so it matches reality brought in by [1]

[1] opendatahub-io/odh-dashboard#270
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved feature/adminui Admin UI Feature kind/enhancement New functionality request (existing augments or new additions) lgtm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable ODH Dashboard Admin & User groups
9 participants