Implement an Admin Flag model and some Emergency Circuit breakers #2967
Conversation
ewdurbin
force-pushed
the
admin_flags
branch
from
b9cf5ad
to
f6f85fc
Compare
warehouse/accounts/views.py
Outdated
| "User Registration Temporarily Disabled", | ||
| queue="error", | ||
| ) | ||
| raise HTTPForbidden() |
There was a problem hiding this comment.
Could be wrong here but I'm not sure the flashed message will appear on the 403 page.
There was a problem hiding this comment.
hrm... yeah. good catch.
There was a problem hiding this comment.
We certainly need to message correctly to users what's up whenever we use these. I'm going to see what options we have.
There was a problem hiding this comment.
Could just redirect to a page that shows the flashed messages (like the index).
| raise _exc_with_message( | ||
| HTTPForbidden, | ||
| ("New Project Registration Temporarily Disabled " | ||
| "See https://pypi.org/help#admin-intervention for details"), |
There was a problem hiding this comment.
Maybe use a request.route_path for the URL here instead?
There was a problem hiding this comment.
probably a good refactor for all of our /help down the line.
ewdurbin
force-pushed
the
admin_flags
branch
from
938726a
to
077b13f
Compare
ewdurbin
changed the title
warehouse/accounts/views.py
Outdated
| @@ -215,6 +216,14 @@ def register(request, _form_class=RegistrationForm): | |||
| if request.authenticated_userid is not None: | |||
| return HTTPSeeOther("/") | |||
|
|
|||
| if AdminFlag().is_enabled(request.db, 'disallow-new-user-registration'): | |||
There was a problem hiding this comment.
You don't need to instantiate AdminFlag here, should just be AdminFlag.is_enabled(...). Same with the other callsite.
There was a problem hiding this comment.
d'oh missed that in factoring to a classmethod. thanks.
|
|
||
|
|
||
| def upgrade(): | ||
| # ### commands auto generated by Alembic - please adjust! ### |
There was a problem hiding this comment.
Not a big deal, but I normally remove these lines, at least as a marker that I looked at the migration and it's doing the right thing.
warehouse/utils/admin_flags.py
Outdated
| enabled = Column(Boolean) | ||
|
|
||
| @classmethod | ||
| def is_enabled(cls, session, flag_name): |
There was a problem hiding this comment.
This should probably either be a static method, or it should use cls in the body instead of AdminFlag.
There was a problem hiding this comment.
yeah, also missed while factoring to a classmethod. do you have a preference between classmethod or static method for this?
There was a problem hiding this comment.
I don't really. I'd maybe use a classmethod just to make inheritance work... but I don't think that really matters since we're never going to use inheritance here anyways.
warehouse/utils/admin_flags.py
Outdated
|
|
||
| id = Column(Text, primary_key=True, nullable=False) | ||
| description = Column(Text, nullable=False) | ||
| enabled = Column(Boolean) |
There was a problem hiding this comment.
Is there a reason to allow this to nullable? What state does that represent? AFAIK there is only two states for a flag, either on or off?
There was a problem hiding this comment.
nope, clear oversight :)
warehouse/utils/admin_flags.py
Outdated
| flag = (session.query(AdminFlag) | ||
| .filter(AdminFlag.id == flag_name) | ||
| .first()) | ||
| if flag is None: |
There was a problem hiding this comment.
I'm torn on this, on one hand it's nice that by surviving a flag not existing in the database, that we're able to handle things like the code running prior to migrations having being ran. However I think that generally code can require a migration being ran-- but migrations have to be compatible with the previous iteration of Warehouse right?
In that case I think I would suggest making it a hard error to not have the flag exist in the database, and prepopulate the database in the migration with any new admin flags. That also leads the way to an eventual admin interface for these, where the admin interface itself doesn't allow creating new admin flags (since they require code support, and getting the ID correct is important) but does allow toggling the flag on/off.It also means there is a single place we can see all of the admin flags (in the DB).
There was a problem hiding this comment.
I'd certainly rather failsafe than make it a hard error if a flag isn't available. Consider the case where a release is being canaried and has optional migrations or works around those in some fashion.
But, I did add the flags we're using into the migration, as indeed it significantly reduces the complexity the Admin interface implementation.
|
|
Allows us to query any flag and it will default to
Falseuntil created and enabled.