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

Private endpoints #2418

Merged
merged 1 commit into from
Apr 4, 2024
Merged

Conversation

itowlson
Copy link
Collaborator

@itowlson itowlson commented Apr 3, 2024

The proposed syntax is that a private endpoint still requires a [[trigger.http]], but has route = false instead of an actual route pattern. (This creates the slight idiosyncrasy that route = false is allowed but route = true is not. I can live with this!)

In addition to unit tests, the front-to-back service chaining test is modified to make the middle component a private endpoint. Let me know if you think further tests are needed or if they should be reorganised to be more specific.

@itowlson itowlson linked an issue Apr 3, 2024 that may be closed by this pull request
@itowlson itowlson force-pushed the private-endpoints branch 3 times, most recently from d410304 to 225a394 Compare April 3, 2024 21:56
@itowlson itowlson marked this pull request as ready for review April 3, 2024 21:56
@itowlson itowlson requested review from lann and fibonacci1729 April 3, 2024 21:56
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
@itowlson itowlson force-pushed the private-endpoints branch from 225a394 to 9fbc065 Compare April 3, 2024 22:05
Copy link
Contributor

@fibonacci1729 fibonacci1729 left a comment

Choose a reason for hiding this comment

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

This looks great to me. I can live with the route = true weirdness too.

Another possible way to express this which we can absolutely ignore that i mentioned if the hate is too strong:

[[trigger.http]]
 # public defaults to `true` to retain `route = "/..."` syntax
route = { path = "/...", public = false }

This has the added benefit of allowing private endpoints to express a route prefix. Admittedly i can't come up with a use-case for this.

Just tossing out low-blood sugar ideas.

@itowlson itowlson requested a review from tschneidereit April 4, 2024 01:20
@itowlson itowlson merged commit 96342e2 into spinframework:main Apr 4, 2024
14 checks passed
@ThorstenHans
Copy link
Contributor

This is great!

Although it is already merged, I would also prefer using a property that reflects visibility as suggested by @fibonacci1729

Some additional ideas:

  • private = true
  • internal = true
  • public = false

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

Successfully merging this pull request may close these issues.

Private endpoints
4 participants