-
Notifications
You must be signed in to change notification settings - Fork 983
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
warehouse: add initial pending OIDC provider models #12572
warehouse: add initial pending OIDC provider models #12572
Conversation
Signed-off-by: William Woodruff <william@trailofbits.com>
__table_args__ = ( | ||
UniqueConstraint( | ||
"repository_name", | ||
"repository_owner", | ||
"workflow_filename", | ||
name="_github_oidc_provider_uc", | ||
), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flagging for review: I had to dupe this UniqueConstraint
between GitHubProvider
and PendingGitHubProvider
, even though they're functionally equivalent, thanks to the unique name
requirement. There might be a better way to do this, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is due to not having set any naming_conventions for this project in the global metadata setup
Line 77 in d58e6b2
metadata = sqlalchemy.MetaData() |
Alembic docs go into it here: https://alembic.sqlalchemy.org/en/latest/naming.html
I think we always named them manually, like here:
warehouse/warehouse/accounts/models.py
Line 197 in d58e6b2
UniqueConstraint("label", "user_id", name="_user_security_keys_label_uc"), |
So it might be a future refactoring to replace the manually-named constraints, but I’d approach that with some serious caution 😉
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall, just the one comment about what should be duplicated vs. shared via mixins.
Signed-off-by: William Woodruff <william@trailofbits.com>
Should be good to go now! |
See #11296.
This is going to be a relatively complex set of models (thanks to the mixins for shared functionality between "pending" and concrete OIDC providers), so I wanted to get them up and merged before beginning on actual routes/view logic.
Needs unit tests.Signed-off-by: William Woodruff william@trailofbits.com