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.

Sign up for GitHub

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

Jump to bottom

OIDC provider ActiveState #13980

Closed
wants to merge 11 commits into from
Closed

OIDC provider ActiveState #13980

wants to merge 11 commits into from

Conversation

th3coop
Copy link
Contributor

@th3coop th3coop commented Jun 21, 2023 •
edited
Loading

1 or 2 PRs to add ActiveState as an OIDC provider. This PR adds the backend features:

  • DB migration
  • associated modeling
  • tests
  • disallow-activestate-oidc admin flag is True

As the OIDC code seems to be evolving, I explicitly avoided making type changes outside the new code as I was getting a lot of merge conflicts otherwise.

ActiveState Internal work tracking ID: DS-1723

@th3coop
Copy link
Contributor Author

th3coop commented Jun 21, 2023

Fixing test coverage. Took longer that I'd care to admit to figure out how to view the coverage report...:D

@th3coop th3coop force-pushed the OIDC-provider-ActiveState branch from e42748f to f328f79 Compare June 21, 2023 19:06
@th3coop
Copy link
Contributor Author

th3coop commented Jun 21, 2023

Self reviewing WIP PR before requesting a review, FYI.

@th3coop th3coop force-pushed the OIDC-provider-ActiveState branch from f328f79 to a1c59fe Compare June 21, 2023 22:24
@th3coop th3coop force-pushed the OIDC-provider-ActiveState branch 7 times, most recently from 578f073 to b8e07b4 Compare June 26, 2023 23:17
@th3coop th3coop marked this pull request as ready for review June 26, 2023 23:22
@th3coop th3coop requested a review from a team as a code owner June 26, 2023 23:22
@th3coop th3coop force-pushed the OIDC-provider-ActiveState branch from b8e07b4 to 82e635a Compare June 26, 2023 23:23
@th3coop
Copy link
Contributor Author

th3coop commented Jun 26, 2023

Ok, I think this PR is where I want it. It's ready for review. Worth noting again (I noted it in the Description as well) that I've disabled the admin flag for ActiveState OIDC for now. I'm not 100% sure that's proper but seemed prudent.

@th3coop th3coop marked this pull request as draft June 28, 2023 16:17
@th3coop
Copy link
Contributor Author

th3coop commented Jun 28, 2023

Sorry for the flip flop, making the PR a Draft again.

I want to run my intended plan by y'all before getting a review:
I want to avoid merging this work into main before we ("we" being ActiveState) have had a chance to test it with a real world use case. That "real world use case" is an internal service that is planned and work will start on it soon. Until that's done, or at least got it to a point where we can test the flow between PyPI and the service that will publish wheel, I'd like to keep this work in a branch.

I'll be starting the front end PR for this today as well.

Does this plan sound ok? I'll take responsibility for keeping the languishing PRs up to date with pypi/warehouse:main, of course.

If an open PR hanging in the repo isn't desirable I can of course close the PR and create a new one later when we're ready to proceed.

Any preference or thoughts on how you'd like me to proceed?

@di
Copy link
Member

di commented Jun 28, 2023

Fine with me to keep this open as a draft!

@th3coop
Copy link
Contributor Author

th3coop commented Jun 29, 2023

Thank you kindly for the quick reply :D

@th3coop th3coop force-pushed the OIDC-provider-ActiveState branch from 248f4ba to 39868f9 Compare July 5, 2023 20:40
@th3coop th3coop force-pushed the OIDC-provider-ActiveState branch from 39868f9 to 47fcf8a Compare July 13, 2023 18:30
@th3coop th3coop force-pushed the OIDC-provider-ActiveState branch from 47fcf8a to 2c61755 Compare July 31, 2023 21:30
@th3coop th3coop force-pushed the OIDC-provider-ActiveState branch 2 times, most recently from 08b826a to 15e7b2d Compare August 8, 2023 22:51
@th3coop th3coop force-pushed the OIDC-provider-ActiveState branch from 15e7b2d to e3e21e6 Compare August 16, 2023 23:09
@th3coop th3coop force-pushed the OIDC-provider-ActiveState branch 2 times, most recently from 2326e31 to 8b2fb62 Compare August 30, 2023 17:24
@th3coop th3coop changed the title OIDC provider active state OIDC provider ActiveState Sep 1, 2023
@th3coop th3coop force-pushed the OIDC-provider-ActiveState branch from 8b2fb62 to 9296139 Compare December 2, 2023 00:14
@woodruffw woodruffw mentioned this pull request Jan 10, 2024
Closed
3 tasks
th3coop and others added 2 commits January 10, 2024 12:50
@th3coop @di
Update warehouse/oidc/models/github.py
b4a30f0 
Co-authored-by: Dustin Ingram <di@users.noreply.github.com>
@th3coop @di
Update warehouse/migrations/versions/9a0ed2044b53_add_activestate_oid…
48c7298 
…c_publisher.py

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>
@th3coop
Copy link
Contributor Author

th3coop commented Jan 10, 2024

Oof. Some silly mistakes in there. Sorry about that @di. I'll make sure the next PRs are more up to snuff.

@th3coop
Copy link
Contributor Author

th3coop commented Jan 10, 2024

Thanks very much for the review though! Working on changes now.

th3coop and others added 4 commits January 10, 2024 13:47
@th3coop @di
Update warehouse/migrations/versions/9a0ed2044b53_add_activestate_oid…
e5ffadf 
…c_publisher.py

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>
@th3coop @di
Update warehouse/migrations/versions/9a0ed2044b53_add_activestate_oid…
a25b7ac 
…c_publisher.py

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>
@th3coop @di
Update warehouse/migrations/versions/9a0ed2044b53_add_activestate_oid…
0234911 
…c_publisher.py

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>
@th3coop @di
Update warehouse/migrations/versions/9a0ed2044b53_add_activestate_oid…
b437d6f 
…c_publisher.py

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>
di
di reviewed Jan 11, 2024
View reviewed changes
@di di mentioned this pull request Jan 11, 2024
Merged
3 tasks
@woodruffw woodruffw self-requested a review January 11, 2024 17:34
@th3coop th3coop force-pushed the OIDC-provider-ActiveState branch from dae2faf to e2ee24f Compare January 11, 2024 19:19
@th3coop th3coop force-pushed the OIDC-provider-ActiveState branch from e2ee24f to bcd580f Compare January 11, 2024 19:23
woodruffw
woodruffw approved these changes Jan 11, 2024
View reviewed changes
Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

LGTM!

@th3coop
Copy link
Contributor Author

th3coop commented Jan 11, 2024

yeah, let me know if you need anything else from me in this PR. I'm currently doing some touch ups on #15168 based on this review. It's almost ready. I'll be pushing the fixes to the tests in a few minutes here I think.

di
di approved these changes Jan 11, 2024
View reviewed changes
Copy link
Member

@di di left a comment

Choose a reason for hiding this comment

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

LGTM! I'll wait to merge this until tomorrow AM.

@di
Copy link
Member

di commented Jan 11, 2024

@th3coop Actually, I don't have a button to bring this branch up-to-date with main, so I'll need you to rebase this for me to be able to merge it.

@th3coop
Copy link
Contributor Author

th3coop commented Jan 12, 2024

No problem @di . Done.

@woodruffw
Copy link
Member

Looks like this needs one last branch update 🙂

@di di mentioned this pull request Jan 12, 2024
Merged
@di
Copy link
Member

di commented Jan 12, 2024

I'll merge this in #15201 instead.

@th3coop, you might want to enable this on future PRs: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

@di di closed this in #15201 Jan 12, 2024
@th3coop th3coop deleted the OIDC-provider-ActiveState branch January 12, 2024 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@di di di approved these changes

@woodruffw woodruffw woodruffw approved these changes

Assignees
No one assigned
Labels
trusted-publishing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@th3coop @di @woodruffw @miketheman