Skip to content

feat: Allow multiple OIDC domains #5210

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
Merged
merged 12 commits into from
Dec 5, 2022
Merged

feat: Allow multiple OIDC domains #5210

merged 12 commits into from
Dec 5, 2022

Conversation

dcarrion87
Copy link
Contributor

No description provided.

@github-actions
Copy link

github-actions bot commented Dec 1, 2022 •
edited
Loading

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@dcarrion87 dcarrion87 changed the title feat: allow multiple oidc domains using comma delimmited list feat: allow multiple oidc domains using comma delimited list Dec 1, 2022
@dcarrion87
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@dcarrion87 dcarrion87 mentioned this pull request Dec 1, 2022
Closed
mafredri
mafredri requested changes Dec 1, 2022
View reviewed changes
Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, this seems like a useful change. Let's make it visible in the config too. 👍🏻

@dcarrion87 dcarrion87 requested a review from mafredri December 1, 2022 13:03
mafredri
mafredri reviewed Dec 1, 2022
View reviewed changes
Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

Nice changes, thanks! Tests are still failing, seems like it's related to the []string change. You'll also want to run make update-golden-files.

@dcarrion87 dcarrion87 requested a review from mafredri December 1, 2022 13:54
mafredri
mafredri approved these changes Dec 1, 2022
View reviewed changes
Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

Looks like you'll need to do a make gen as well, but other than that, this looks great!

bpmct
bpmct requested changes Dec 1, 2022
View reviewed changes
Copy link
Member

@bpmct bpmct left a comment

Choose a reason for hiding this comment

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

@dcarrion87 - thanks for the contribution! We'll need to make some edits to the docs and understand the impact of the breaking change before merging, but other than that it LGTM

@dcarrion87
Copy link
Contributor Author

dcarrion87 commented Dec 1, 2022 •
edited
Loading

Looks like you'll need to do a make gen as well, but other than that, this looks great!

No changes were generated when doing a make gen

@mafredri
Copy link
Member

mafredri commented Dec 1, 2022

No changes were generated when doing a make gen

Hmm, that is unexpected. See https://github.com/coder/coder/actions/runs/3593197414/jobs/6050208601 for what’s wrong. You can fix it manually too, or delete the file and re-run make gen.

I’ll investigate to see if I can figure out why it didn’t pick up on the changes.

@dcarrion87 dcarrion87 requested a review from a team as a code owner December 1, 2022 22:01
@dcarrion87 dcarrion87 requested review from BrunoQuaresma and removed request for a team December 1, 2022 22:01
@BrunoQuaresma BrunoQuaresma removed their request for review December 2, 2022 13:52
@dcarrion87 dcarrion87 requested review from mafredri and removed request for mafredri December 2, 2022 13:53
@dcarrion87 dcarrion87 requested review from bpmct and mafredri and removed request for bpmct December 2, 2022 13:53
mafredri
mafredri approved these changes Dec 2, 2022
View reviewed changes
Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

Looks great, just need to get the tests passing (right now their flaking all on their own, no issues with the PR yet anyway).

@dcarrion87 dcarrion87 requested a review from bpmct December 3, 2022 00:16
@bpmct bpmct mentioned this pull request Dec 5, 2022
Closed
@mafredri mafredri changed the title feat: allow multiple oidc domains using comma delimited list feat: Allow multiple OIDC domains Dec 5, 2022
@mafredri mafredri merged commit 061635c into coder:main Dec 5, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Dec 5, 2022
@mafredri
Copy link
Member

mafredri commented Dec 5, 2022

Big thanks @dcarrion87 for all your work on this issue!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mafredri mafredri mafredri approved these changes

@bpmct bpmct bpmct approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dcarrion87 @mafredri @bpmct