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

Read only AdminFlag #3393

Merged
merged 9 commits into from
Mar 23, 2018
Merged

Read only AdminFlag #3393

merged 9 commits into from
Mar 23, 2018

Conversation

di
Copy link
Member

@di di commented Mar 23, 2018

Fixes #164.

  • Adds a read-only admin flag which dooms the transaction
  • Adds request methods for admin flags (request.flags.enabled and request.flags.all)
  • Adds a AdminFlag.notify attribute to determine if the flag should be displayed as a notification bar
  • Shows a notification bar for any notifiable admin flags
  • Admin UI for editing flags

@di di requested a review from ewdurbin March 23, 2018 21:33
Copy link
Member

@ewdurbin ewdurbin left a comment

Choose a reason for hiding this comment

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

This is great! Only one concern, but I'm OK either way.

warehouse/db.py Outdated
# Check if we're in read-only mode
from warehouse.admin.flags import AdminFlag
flag = session.query(AdminFlag).get('read-only')
if flag and flag.enabled:
Copy link
Member

Choose a reason for hiding this comment

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

can we get an overall admin user short circuit here as well? that would alleviate the need to except for this in edit_flag and avoid having to except this for each and every operation an admin may need to perform during read-only situations.

flag.description = request.POST['description']
flag.enabled = bool(request.POST.get('enabled'))

if flag.id == 'read-only' and request.tm.isDoomed():
Copy link
Member

Choose a reason for hiding this comment

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

not sure having to add this clause for every admin action we add down the line is a a great approach. suggest we do this once in warehouse.db

@dstufft
Copy link
Member

dstufft commented Mar 23, 2018

What's the behavior that users are going to see if they attempt to do something that writes to the DB when we're in read only mode?

@di
Copy link
Member Author

di commented Mar 23, 2018

@dstufft It will look like the write has succeeded, but it won't, and there will be a big red notification banner saying "Read Only Mode: Any write operations will have no effect"

We probably need to add a special case to fail upload since twine users won't see the notification banner.

@di di merged commit a6d3418 into pypi:master Mar 23, 2018
@di di deleted the read-only-adminflag branch March 23, 2018 22:43
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.

3 participants