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

Fix serving runtime edit creation #1291

Conversation

lucferbux
Copy link
Contributor

Description

Closes #1259

How Has This Been Tested?

Regular creation

Test if an admin user can create a model server

  1. Create a new project
  2. Create a model server with authentication enabled
  3. Create some secrets
  4. Check that is working as expected

Edit user

Test if a user with edit permission can create a model server

  1. Create a new project
  2. Add a user without cluster permissions with edit role in the project sharing panel
  3. Log in as this regular user
  4. Create a new model server
  5. You should see authentication disabled
  6. You should be able to create a model server
Screenshot 2023-05-25 at 22 18 43

Edit user edit project

Test if a user with edit permission can edit a deployed model server

  1. Create a new project
  2. Create a new model server with auth
  3. Add a user without cluster permissions with edit role in the project sharing panel
  4. Log in as this regular user
  5. Edit the created model server
  6. You should see authentication disabled
  7. You should be able to edit and update the model server
Screenshot 2023-05-25 at 22 18 59

Test Impact

All the coverage of the testing will be covered in the testing efforts for the model serving feature

Request review criteria:

  • The commits have meaningful messages (squashes happen on merge by the bot).
  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change.
  • 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
  • The developer has added tests or explained why testing cannot be added (unit tests & storybook for related changes)

@lucferbux lucferbux requested review from andrewballantyne and Gkrumbach07 and removed request for alexcreasy May 25, 2023 20:19
@lucferbux
Copy link
Contributor Author

@vconzola Let me know if this is the behavior you were expecting.
Also, no idea what to put in the warning, can you give me some feedback there?

@lucferbux lucferbux force-pushed the fix-serving-runtime-edit-creation branch from bc97cec to f1ae4f7 Compare May 25, 2023 20:21
@Gkrumbach07
Copy link
Member

tested and admin regular user cannot use the checkbox

/lgtm

Copy link
Member

@DaoDaoNoCode DaoDaoNoCode left a comment

Choose a reason for hiding this comment

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

lgtm

@vconzola
Copy link

vconzola commented Jun 1, 2023

@lucferbux I know we talked about keeping the warning, at least for now. But did we talk about the actual text of the message? Can you make it more specific and say, "Administrator permissions in this namespace are required to generate tokens." if that's technically correct. If that's not technically correct, what is?

@DaoDaoNoCode
Copy link
Member

/hold

@openshift-ci openshift-ci bot added do-not-merge/hold This PR is hold for some reason and removed lgtm labels Jun 1, 2023
@lucferbux lucferbux removed the do-not-merge/hold This PR is hold for some reason label Jun 2, 2023
@lucferbux
Copy link
Contributor Author

@vconzola Done, let me know what you think so we can merge it asap!

@lucferbux lucferbux force-pushed the fix-serving-runtime-edit-creation branch from 535f173 to 3f105f7 Compare June 2, 2023 09:35
@lucferbux
Copy link
Contributor Author

@DaoDaoNoCode can you re-review it once @vconzola take a look?

@lucferbux lucferbux force-pushed the fix-serving-runtime-edit-creation branch from 3f105f7 to 41d34b2 Compare June 2, 2023 09:48
@lucferbux
Copy link
Contributor Author

@kywalker-rh can you take a look please?

@openshift-ci openshift-ci bot added the lgtm label Jun 2, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 2, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, DaoDaoNoCode, Gkrumbach07

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:

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

Any UX comments should be caught up after the fact. Today is release day, this doesn't need to wait.

@openshift-merge-robot openshift-merge-robot merged commit d6f0f68 into opendatahub-io:main Jun 2, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Users with Edit permissions cannot create a model server runtime in a shared project
6 participants