server: Drop HTML index page#8478
server: Drop HTML index page#8478johanfylling wants to merge 4 commits intoopen-policy-agent:mainfrom
Conversation
Fixes: open-policy-agent#8477 Signed-off-by: Johan Fylling <johan.dev@fylling.se>
| mainRouter.Handle("GET /v1/compile/{path...}", s.instrumentHandler(s.v1CompileFilters, PromHandlerV1Compile)) | ||
| mainRouter.Handle("GET /v1/config", s.instrumentHandler(s.v1ConfigGet, PromHandlerV1Config)) | ||
| mainRouter.Handle("GET /v1/status", s.instrumentHandler(s.v1StatusGet, PromHandlerV1Status)) | ||
| mainRouter.Handle("POST /{$}", s.instrumentHandler(s.unversionedPost, PromHandlerIndex)) |
There was a problem hiding this comment.
Should we drop the unversioned query endpoint too?
This one might actually be used.
There was a problem hiding this comment.
Yeah "webhook" usage, potentially.
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| \ \_______\ \ \__\ \ \__\ \__\ | ||
| \|_______| \|__| \|__|\|__| | ||
| </pre> | ||
| Open Policy Agent - An open source project to policy-enable your service.<br> |
There was a problem hiding this comment.
I suppose we could also keep this, and just drop the query form (and maybe the info fields) below ..
There was a problem hiding this comment.
What do we gain? We're only telling the world what this server is. I don't think there's any benefit here.
There was a problem hiding this comment.
True. Looks like we have some failing tests because of dropping the endpoint, so maybe someone else is using it for health/liveliness checks despite there being dedicated endpoints for that 🤷.
There was a problem hiding this comment.
The health endpoint isn't exactly new, so I think the empty-body 200 is a good compromise.
srenatus
left a comment
srenatus
left a comment
There was a problem hiding this comment.
LGTM. Do you think we should solicit some more opinions?
|
Yea, let's let this one bake for a bit 👍. I just wanted to get a PR up so it doesn't fall to the wayside. We have a month until the next release 🙂. |
|
For better or worse, I have seen many use this as a "is OPA up?" endpoint. I'd suggest that if we do remove the HTML form page (and I'll be pouring one out for you, old friend) we keep the endpoint just serving a static 200 OK or so, at least until the next major version bump. |
|
@anderseknert , yeah, let's do that 👍. We even have some tests using this for status checks ourselves, it would seem 😄. |
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
v1/server/server.go
Outdated
| } | ||
|
|
||
| func (*Server) indexGet(w http.ResponseWriter, _ *http.Request) { | ||
| w.WriteHeader(http.StatusOK) |
There was a problem hiding this comment.
[nit] I think that's what stdlib will do if we just do nothing here?
There was a problem hiding this comment.
Then we get a 405 Method Not Allowed. So if that's the default behavior we're doing something to disable it.
| return | ||
| } | ||
|
|
||
| w.WriteHeader(http.StatusNoContent) |
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
| Build Commit: {{ .BuildCommit }}<br> | ||
| Build Timestamp: {{ .BuildTimestamp }}<br> | ||
| Build Hostname: {{ .BuildHostname }}<br> | ||
| <br> |
There was a problem hiding this comment.
I agree that removing the form makes sense, but I might vote to keep the Commit and Version as I am unsure how you might do this without running rego otherwise and people might depend on that if not the form?
- We can remove the form. Add deprecation text to page.
- Still 200, but remove the version etc.
- Remove the page. but still show a 200OK and
{"status": "OK"}
There was a problem hiding this comment.
I'm ok with only dropping the form 👍.
Looks like maybe only the version is part of the /status endpoint's returned labels; maybe we should add these other fields too(?).
There was a problem hiding this comment.
Yeah I think that makes sense as a continuity gesture at least. It doesn't look like it's possible to change the version label or the id one either, would these become the same? it might be that someone is using hostname in their own labels.
but no query form. Signed-off-by: Johan Fylling <johan.dev@fylling.se>
4 participants
Fixes: #8477