feat: Add all json schemas endpoint #1703

provokateurin · 2021-08-30T14:05:06Z

This PR implements an endpoint to list all Json schemas

Related issue(s)

#1699

Checklist

I have read the contributing guidelines.
I am following the
contributing code guidelines.
I have read the security policy.
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature
works.
I have added or changed the documentation.

CLAassistant · 2021-08-30T14:05:11Z

All committers have signed the CLA.

provokateurin · 2021-08-30T14:05:34Z

I'm not sure which docs I'm supposed to update and also if I should commit the generated code.

schema/handler.go

aeneasr

Awesome, thank you for your contribution! This looks pretty good and I have some ideas how to improve it further :)

schema/handler.go

codecov · 2021-08-30T14:36:48Z

Codecov Report

Merging #1703 (6a4a83f) into master (5b456b3) will decrease coverage by 0.04%.
The diff coverage is 80.89%.

@@            Coverage Diff             @@
##           master    #1703      +/-   ##
==========================================
- Coverage   74.10%   74.06%   -0.05%     
==========================================
  Files         260      260              
  Lines       12715    12770      +55     
==========================================
+ Hits         9423     9458      +35     
- Misses       2667     2681      +14     
- Partials      625      631       +6

Impacted Files	Coverage Δ
selfservice/strategy/link/strategy_recovery.go	`67.92% <69.23%> (+0.91%)`	⬆️
schema/schema.go	`86.66% <77.77%> (-2.23%)`	⬇️
schema/handler.go	`84.48% <92.68%> (-8.86%)`	⬇️
cmd/courier/watch.go	`60.34% <0.00%> (-12.07%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 131e62e...6a4a83f. Read the comment docs.

provokateurin · 2021-08-30T18:28:35Z

@aeneasr i hope this is to your liking

aeneasr · 2021-08-31T06:45:48Z

Thank you! It looks like the tests fail with a panic right now:

https://app.circleci.com/pipelines/github/ory/kratos/4621/workflows/b18b066e-0dab-4771-8311-79846e0692fe/jobs/25049

Could you please take a look? :)

provokateurin · 2021-08-31T07:47:48Z

Uh the endpoint returns the same schema multiple times. I need to fix that.

provokateurin · 2021-08-31T08:27:23Z

I think the implementation should be fixed now. I also improved the test, because the bug slipped past it. Sorry for the many force pushes :(

provokateurin · 2021-08-31T08:41:58Z

The test fail seems completely unrelated to this PR

provokateurin · 2021-08-31T09:13:14Z

@aeneasr i think there is a but in the admin list identities endpoint. requesting page 0 and page 1 both return the same results. In this endpoint pages start at 0 and the bug is not present. this should be consistent though and the spec says pages start at 0, so the admin list identities endpoint needs to be fixed.

zepatrik

Thanks for updating the PR 👍
I have some ideas how to improve the code even further 😉

schema/handler.go

schema/schema.go

schema/handler.go

provokateurin · 2021-09-01T08:17:58Z

@zepatrik i applied all your suggestions

zepatrik

Nice, this is going in a very good direction 😉
Just some final details, then it should be good to merge 👍

schema/handler.go

schema/schema_test.go

schema/handler_test.go

schema/handler.go

schema/handler_test.go

provokateurin · 2021-09-02T09:57:55Z

I hope this is all good now

zepatrik

Nice, nearly there 👍

schema/handler.go

schema/handler_test.go

provokateurin · 2021-09-06T08:27:06Z

@zepatrik i don't know how the hot-reloading of the config should work, can you give me a hint?

zepatrik · 2021-09-06T08:28:37Z

What you did should work already, isn't it?

provokateurin · 2021-09-06T08:29:20Z

yes it works, but it's a bit hacky

provokateurin · 2021-09-06T08:29:47Z

circleci test fails because of something unrelated with goreleaser

zepatrik · 2021-09-06T08:35:49Z

You have to merge/rebase to current master 😉 #1730
Btw, you don't have to force push all the time, we squash the commits in the end anyway. You can push later changes as individual commits.

provokateurin · 2021-09-06T08:38:32Z

Ok :) I wasn't sure what the merge strategy is, so I didn't want to spam the git log

provokateurin · 2021-09-06T09:11:16Z

the failed test to me seems unrelated to these changes

schema/handler_test.go

provokateurin · 2021-09-07T09:47:41Z

@zepatrik i fixed the coverage as you suggested. the remaining uncovered lines are all error handling. i don't know if that is something that should be covered, because it wasn't covered before too.

zepatrik · 2021-09-08T15:13:50Z

That is fine, only errors that are expected to occur during normal usage have to be covered. These are not expected errors.

aeneasr · 2021-09-09T07:55:01Z

@zepatrik green light?

aeneasr

Just some swagger stuff :)

schema/handler.go

zepatrik

Nice, looks good to me 😉 Thanks for all the hard work 👍

provokateurin · 2021-09-09T10:11:46Z

Thanks for all your nice comments :) It was a pleasure to work with so friendly developers

provokateurin · 2021-09-11T08:25:13Z

When will this be merged/released?

zepatrik · 2021-09-13T12:07:51Z

I am waiting for @aeneasr to approve.

aeneasr

Thank you ! Looking great and also awesome tests 😎 You’re almost there!

schema/handler.go

schema/schema.go

provokateurin · 2021-09-20T09:30:42Z

@aeneasr anything missing for the merge?

aeneasr · 2021-09-21T08:12:51Z

Just got back from holidays. Will review as soon as possible!

aeneasr

Awesome, thank you! 🎉 Your contribution makes Ory better :)

provokateurin requested review from aeneasr and zepatrik as code owners August 30, 2021 14:05

aeneasr reviewed Aug 30, 2021

View reviewed changes

schema/handler.go Outdated Show resolved Hide resolved

aeneasr requested changes Aug 30, 2021

View reviewed changes

schema/handler.go Outdated Show resolved Hide resolved

provokateurin force-pushed the feature/schema-ids-endpoint branch from 62d97fb to bbb7a17 Compare August 30, 2021 18:28

provokateurin force-pushed the feature/schema-ids-endpoint branch from bbb7a17 to b97254e Compare August 30, 2021 18:41

provokateurin force-pushed the feature/schema-ids-endpoint branch from b97254e to 84e01ce Compare August 31, 2021 07:00

provokateurin changed the title ~~feat: Add json schema ids endpoint~~ feat: Add all json schemasendpoint Aug 31, 2021

provokateurin changed the title ~~feat: Add all json schemasendpoint~~ feat: Add all json schemas endpoint Aug 31, 2021

provokateurin force-pushed the feature/schema-ids-endpoint branch from 84e01ce to d6466fc Compare August 31, 2021 08:24

zepatrik reviewed Aug 31, 2021

View reviewed changes

schema/handler.go Outdated Show resolved Hide resolved

schema/schema.go Outdated Show resolved Hide resolved

schema/handler.go Outdated Show resolved Hide resolved

schema/handler.go Outdated Show resolved Hide resolved

provokateurin force-pushed the feature/schema-ids-endpoint branch from d6466fc to 7b95b1a Compare September 1, 2021 08:17

zepatrik requested changes Sep 1, 2021

View reviewed changes

provokateurin force-pushed the feature/schema-ids-endpoint branch from 7b95b1a to e0c4e48 Compare September 2, 2021 09:57

zepatrik requested changes Sep 2, 2021

View reviewed changes

schema/handler.go Outdated Show resolved Hide resolved

schema/handler.go Show resolved Hide resolved

schema/handler_test.go Outdated Show resolved Hide resolved

provokateurin force-pushed the feature/schema-ids-endpoint branch from e0c4e48 to 4720dc8 Compare September 6, 2021 08:26

provokateurin force-pushed the feature/schema-ids-endpoint branch from 4720dc8 to 874dc1b Compare September 6, 2021 08:38

zepatrik reviewed Sep 6, 2021

View reviewed changes

schema/handler_test.go Outdated Show resolved Hide resolved

aeneasr requested changes Sep 9, 2021

View reviewed changes

schema/handler.go Outdated Show resolved Hide resolved

schema/handler.go Outdated Show resolved Hide resolved

schema/handler.go Outdated Show resolved Hide resolved

schema/handler.go Outdated Show resolved Hide resolved

schema/handler.go Outdated Show resolved Hide resolved

provokateurin force-pushed the feature/schema-ids-endpoint branch from dd1ff61 to 6880efc Compare September 9, 2021 09:34

zepatrik approved these changes Sep 9, 2021

View reviewed changes

aeneasr requested changes Sep 14, 2021

View reviewed changes

schema/handler.go Outdated Show resolved Hide resolved

schema/handler.go Outdated Show resolved Hide resolved

schema/handler.go Outdated Show resolved Hide resolved

schema/handler.go Outdated Show resolved Hide resolved

schema/schema.go Show resolved Hide resolved

provokateurin force-pushed the feature/schema-ids-endpoint branch from 6880efc to 2517dff Compare September 16, 2021 07:35

Kate Döen added 4 commits September 23, 2021 14:14

feat: Add all json schemas endpoint

c2477ea

Set schemas in tests in a less hacky way

be0f183

Fix swagger docs

2d1a2b0

Fix last review comments

6a4a83f

provokateurin force-pushed the feature/schema-ids-endpoint branch from 2517dff to 6a4a83f Compare September 23, 2021 12:14

aeneasr approved these changes Sep 23, 2021

View reviewed changes

aeneasr merged commit aa23d5d into ory:master Sep 23, 2021

provokateurin deleted the feature/schema-ids-endpoint branch September 27, 2021 08:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add all json schemas endpoint #1703

feat: Add all json schemas endpoint #1703

provokateurin commented Aug 30, 2021 •

edited

CLAassistant commented Aug 30, 2021 •

edited

provokateurin commented Aug 30, 2021

aeneasr left a comment

codecov bot commented Aug 30, 2021 •

edited

provokateurin commented Aug 30, 2021

aeneasr commented Aug 31, 2021

provokateurin commented Aug 31, 2021

provokateurin commented Aug 31, 2021

provokateurin commented Aug 31, 2021

provokateurin commented Aug 31, 2021

zepatrik left a comment

provokateurin commented Sep 1, 2021

zepatrik left a comment

provokateurin commented Sep 2, 2021

zepatrik left a comment

provokateurin commented Sep 6, 2021

zepatrik commented Sep 6, 2021

provokateurin commented Sep 6, 2021

provokateurin commented Sep 6, 2021

zepatrik commented Sep 6, 2021

provokateurin commented Sep 6, 2021

provokateurin commented Sep 6, 2021

provokateurin commented Sep 7, 2021

zepatrik commented Sep 8, 2021

aeneasr commented Sep 9, 2021

aeneasr left a comment

zepatrik left a comment

provokateurin commented Sep 9, 2021

provokateurin commented Sep 11, 2021

zepatrik commented Sep 13, 2021

aeneasr left a comment

provokateurin commented Sep 20, 2021

aeneasr commented Sep 21, 2021

aeneasr left a comment

feat: Add all json schemas endpoint #1703

feat: Add all json schemas endpoint #1703

Conversation

provokateurin commented Aug 30, 2021 • edited

Related issue(s)

Checklist

CLAassistant commented Aug 30, 2021 • edited

provokateurin commented Aug 30, 2021

aeneasr left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 30, 2021 • edited

Codecov Report

provokateurin commented Aug 30, 2021

aeneasr commented Aug 31, 2021

provokateurin commented Aug 31, 2021

provokateurin commented Aug 31, 2021

provokateurin commented Aug 31, 2021

provokateurin commented Aug 31, 2021

zepatrik left a comment

Choose a reason for hiding this comment

provokateurin commented Sep 1, 2021

zepatrik left a comment

Choose a reason for hiding this comment

provokateurin commented Sep 2, 2021

zepatrik left a comment

Choose a reason for hiding this comment

provokateurin commented Sep 6, 2021

zepatrik commented Sep 6, 2021

provokateurin commented Sep 6, 2021

provokateurin commented Sep 6, 2021

zepatrik commented Sep 6, 2021

provokateurin commented Sep 6, 2021

provokateurin commented Sep 6, 2021

provokateurin commented Sep 7, 2021

zepatrik commented Sep 8, 2021

aeneasr commented Sep 9, 2021

aeneasr left a comment

Choose a reason for hiding this comment

zepatrik left a comment

Choose a reason for hiding this comment

provokateurin commented Sep 9, 2021

provokateurin commented Sep 11, 2021

zepatrik commented Sep 13, 2021

aeneasr left a comment

Choose a reason for hiding this comment

provokateurin commented Sep 20, 2021

aeneasr commented Sep 21, 2021

aeneasr left a comment

Choose a reason for hiding this comment

provokateurin commented Aug 30, 2021 •

edited

CLAassistant commented Aug 30, 2021 •

edited

codecov bot commented Aug 30, 2021 •

edited