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

delete non-empty IdPs #1068

Merged
merged 3 commits into from
Apr 21, 2020
Merged

delete non-empty IdPs #1068

merged 3 commits into from
Apr 21, 2020

Conversation

fisx
Copy link
Contributor

@fisx fisx commented Apr 21, 2020 •
edited

https://github.com/zinfra/backend-issues/issues/1295

This is based on #1065, I can rebase this when that is merged.

@fisx
Copy link
Contributor Author

fisx commented Apr 21, 2020

nope, not yet working, fix coming up.

@fisx
Copy link
Contributor Author

fisx commented Apr 21, 2020

@arianvp you have the most / freshest context, perhaps you can take a look at this after #1065?

@fisx
Copy link
Contributor Author

fisx commented Apr 21, 2020 •
edited

tests have passed locally. i'll ping ci when #1063 is green again.

ah, there it is. never mind. :)

arianvp
arianvp reviewed Apr 21, 2020
View reviewed changes
services/spar/test-integration/Util/Core.hs
getUserBrig uid = do
env <- ask
let req =
(env ^. teBrig) . path "/self"
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer if we stop calling "user-facing" endpoints from service to service. We should instead only use internal endpoints between services

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, i agreed with you when you wrote this, but i just realized that luckily this is not service-to-server, but integration-test-to-service. i suppose that's legit?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well it's copied from a place in spar which actually is service-to-service :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah. right. so, i'm happy to take on the task of refactoring this properly, over our entire code base. but that's a separate concern, and the helper in the tests can stay! :)

arianvp
arianvp approved these changes Apr 21, 2020
View reviewed changes
Copy link
Contributor

@arianvp arianvp left a comment

Choose a reason for hiding this comment

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

Given the comments, approved

@fisx fisx changed the title delete non-empty IdPs [WIP] delete non-empty IdPs Apr 21, 2020
@fisx fisx changed the base branch from fisx/spar-update-issuer-v1 to develop April 21, 2020 17:30
@fisx fisx changed the title [WIP] delete non-empty IdPs delete non-empty IdPs Apr 21, 2020
@fisx fisx merged commit 29de94b into develop Apr 21, 2020
@fisx fisx deleted the fisx/spar-delete-idp branch April 21, 2020 18:29
@fisx fisx mentioned this pull request Apr 21, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arianvp arianvp arianvp 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.

None yet
2 participants
@fisx @arianvp