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.

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

OpenAPI docs #40162

Merged
merged 7 commits into from
Mar 20, 2024
Merged

OpenAPI docs #40162

merged 7 commits into from
Mar 20, 2024

Conversation

piranha
Copy link
Contributor

@piranha piranha commented Mar 15, 2024

Adds OpenAPI generation for defendpoint and an interface for it at /api/docs/.

Caveats:

  • I don't understand why clj-kondo refuses to see those 4 endpoints by name (GET_docs* mentioned in api.routes and others I'm using for tests)
  • There is no security scheme defined, so it says "Authentication: Not Required" everywhere (see screenshot)
  • If we have POST/PUT requests having query parameters, we need some way of differentiating them
  • Lots of fields are required, but ... or null - that needs to improve

I'd really love to fix clj-kondo, but don't understand how. I think others are fine to fix later on when we get a bit of feedback on this.

image

@piranha piranha added the no-backport Do not backport this PR to any branch label Mar 15, 2024
@piranha piranha requested a review from escherize March 15, 2024 11:29
@piranha piranha requested a review from camsaul as a code owner March 15, 2024 11:29
@metabase-bot metabase-bot bot added the .Team/BackendComponents also known as BEC label Mar 15, 2024
@piranha piranha force-pushed the openapi branch 2 times, most recently from d54f56f to 14177fb Compare March 15, 2024 11:38
Copy link

replay-io bot commented Mar 15, 2024

Status Complete ↗︎
Commit 8206707
Results
⚠️ 7 Flaky
2365 Passed

Copy link
Contributor

@escherize escherize left a comment

Choose a reason for hiding this comment

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

It looks good, and I like the tests, but I'm not sure what the exact goal is, could you put in link to the issue so I can know exactly what this is solving?

I am also lacking some context for parsing out what a few of your pr's comments mean:

  • I don't understand why clj-kondo refuses to see those 4 endpoints by name

Which 4?

  • There is no security scheme defined, so it says "no authentication" everywhere

Do we need to define one? What's everywhere?

  • If we have POST/PUT requests having query parameters, we need some way of differentiating them

Is this going to be some followup work?

(premium-features/assert-has-feature feature feature-name)
(handler request respond raise)))
(with-meta
(fn [request respond raise]
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw a weird thread on clojurians slack about functions not "technically" having support for metadata, and that it's "an implementation detail". I think this is fine though, I doubt they'll ever change it. thread here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uhhh weird right?! Anyway, I think we don't have too many options here - operating vars in this place would weird.

src/metabase/api/automagic_dashboards.clj Outdated Show resolved Hide resolved
src/metabase/api/automagic_dashboards.clj Outdated Show resolved Hide resolved
src/metabase/api/automagic_dashboards.clj Outdated Show resolved Hide resolved
src/metabase/api/automagic_dashboards.clj Outdated Show resolved Hide resolved
src/metabase/api/automagic_dashboards.clj Show resolved Hide resolved
@piranha
Copy link
Contributor Author

piranha commented Mar 15, 2024

Is this going to be some followup work?

Yeah, I think so. Making this perfect will take lots of time - meanwhile it's going to be useful as it is.

Copy link
Contributor

@escherize escherize left a comment

Choose a reason for hiding this comment

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

If we're good with follow up work, let's merge it and keep adding to it along the way: 👍

@piranha
Copy link
Contributor Author

piranha commented Mar 19, 2024

Right... I still need to understand why clj-kondo is unhappy. 😁

@piranha piranha force-pushed the openapi branch 5 times, most recently from 4205bb9 to 68c8017 Compare March 20, 2024 13:37
@piranha piranha merged commit 22e3963 into master Mar 20, 2024
110 checks passed
@piranha piranha deleted the openapi branch March 20, 2024 17:22
Copy link

@piranha Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

@piranha piranha added this to the 0.50 milestone Mar 20, 2024
calherries pushed a commit that referenced this pull request Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport Do not backport this PR to any branch .Team/BackendComponents also known as BEC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants