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

chore: release @credo-ts/rest 0.10.0 #240

Merged
merged 2 commits into from
Mar 31, 2024
Merged

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Feb 14, 2024

🤖 I have created a release *beep* *boop*

0.10.0 (2024-03-31)

⚠ BREAKING CHANGES

Features

Bug Fixes

Miscellaneous Chores


This PR was generated with Release Please. See documentation.

Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

First need to update to Credo 0.5

@github-actions github-actions bot force-pushed the release-rest-v0.10.0 branch 3 times, most recently from 6a8334c to 4e66138 Compare March 30, 2024 22:42
Copy link
Contributor

@genaris genaris left a comment

Choose a reason for hiding this comment

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

The only thing I don't like is the 'trick' of using @Security decorators to use authorization headers for tenants when they aren't actually meant for auth, but I guess it's something we should live with due to Swagger/TSOA (or their combination) limitations, right?

@TimoGlastra
Copy link
Contributor

I think implementing it as auth in TSOA is fine as it's really only internal. Or do you also dislike that?

Or is it the header? I do think a header is the simplest approach but I'm open to other solutions.

Please let me know which parts you don't like (x-tenant-id header, using tsoa authentication logic for this (which is used internally), or the security mention in the swagger UI). Because all of them can be solved independently of the others

@genaris
Copy link
Contributor

genaris commented Mar 31, 2024

I think implementing it as auth in TSOA is fine as it's really only internal. Or do you also dislike that?

Or is it the header? I do think a header is the simplest approach but I'm open to other solutions.

Please let me know which parts you don't like (x-tenant-id header, using tsoa authentication logic for this (which is used internally), or the security mention in the swagger UI). Because all of them can be solved independently of the others

Yeah using the parameter from the header is absolutely fine. I was referring mostly to the fact of calling it 'auth' because it can lead to some confusions, like the question raised by Warren when you presented this in last Credo contributors call.

In practice, as you say, it is just something noticeable in Swagger UI, so it's not that important. Maybe the tenants header could be handled by a @Middleware, but I don't know if it will be easily possible to specify the header parameter in all routes as it is when using securityDefinitions.

@TimoGlastra
Copy link
Contributor

@genaris I've made improvements in #263, please let me know what you think! I can make ammendments in a follow up version if you would like to see it differently. The behaviour in general stays the same: x-tenant-id header to specify which tenant. But we can tweak how it's shown in SwaggerUI/openapi.json as well as how it works internally (kept it as authentication for now as it applies perfectly for what we're trying to do)

Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link

codecov bot commented Mar 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.95%. Comparing base (658cadc) to head (2aa6451).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #240   +/-   ##
=======================================
  Coverage   57.95%   57.95%           
=======================================
  Files          95       95           
  Lines        2528     2528           
  Branches      513      513           
=======================================
  Hits         1465     1465           
  Misses        983      983           
  Partials       80       80           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TimoGlastra TimoGlastra merged commit ee41acc into main Mar 31, 2024
10 checks passed
@TimoGlastra TimoGlastra deleted the release-rest-v0.10.0 branch March 31, 2024 14:22
Copy link
Contributor Author

@genaris
Copy link
Contributor

genaris commented Mar 31, 2024

Nicely done, @TimoGlastra !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants