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

Adding /version route. Fixes #2914 #4059

Merged
merged 7 commits into from
Oct 24, 2023
Merged

Adding /version route. Fixes #2914 #4059

merged 7 commits into from
Oct 24, 2023

Conversation

dessalines
Copy link
Member

@dessalines dessalines commented Oct 18, 2023

I'll add this to the lemmy-ansible nginx also.

@Nutomic
Copy link
Member

Nutomic commented Oct 19, 2023

It seems a bit misleading to call it /version and then return stats about the number of users. If we do it like this, might as well be a redirect. I checked some other websites but didnt find any with this /version, can you show some examples?

@MV-GH @buresdv You can see the data format this would return here. Is that what you expect, or rather a plain version string?

@MV-GH
Copy link
Contributor

MV-GH commented Oct 19, 2023

Well originally, before I knew about nodeinfo, i wanted an endpoint that just returns version.

But from experience, I know that people sometimes try other activitypub software in lemmy clients. So now I can handle that.

So the only problem I had was discoverability of this endpoint. If version only returns version but also documents the above endpoint that's absolutely fine for me.

@dessalines
Copy link
Member Author

I can totally change it to a plain version string. I'd just thought that extra info might be useful, especially the software one.

@Nutomic
Copy link
Member

Nutomic commented Oct 20, 2023

@MV-GH mentions that the problem is with discovery of the endpoint. Based on that, I think a redirect would be the best solution.

@MV-GH
Copy link
Contributor

MV-GH commented Oct 20, 2023

If this endpoint documents the other too, Then we have best of both worlds. It will then automatically be pulled into my openapi spec

Don't see much value in having two endpoints that return the same data

@dessalines
Copy link
Member Author

So just an nginx redirect to the nodeinfo endpoint?

@dessalines dessalines marked this pull request as draft October 20, 2023 14:03
@Nutomic
Copy link
Member

Nutomic commented Oct 20, 2023

Yes

@dessalines dessalines marked this pull request as ready for review October 20, 2023 15:08
@Nutomic
Copy link
Member

Nutomic commented Oct 20, 2023

Like I said in the ansible PR, I would be fine with doing the redirect in Rust. Misread your previous comment on that.

@dessalines
Copy link
Member Author

That's what I had it as originally, version and nodeinfo being the same route. Lets just go with the nginx one I think.

@Nutomic
Copy link
Member

Nutomic commented Oct 23, 2023

Im not talking about two different endpoints which return the same data, but a redirect with HTTP status 30x.

https://docs.rs/actix-web/latest/actix_web/web/struct.Redirect.html

@Nutomic Nutomic merged commit 1596aee into main Oct 24, 2023
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.

3 participants