fix: admin REST API to be enabled only if config is set

[chaitanyaprem]

Description

Admin REST API was enabled even if restadmin flag was set to false.

Note: Tested by running node locally and enabling/disabling the config flag to verify the availability of Admin REST API.

Changes

• Included config check before installing Admin REST API handlers

[github-actions]

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2218

Built from b22cdc4

[NagyZoltanPeter]

I have some generic thoughts on this PR.
One littel, and yes it is a general notice to myself also as me also running into this time-to-time. It is better avoid general formatting changes when having a fix PR. Maybe you have some auto-formatting enabled, idk. But it makes harder to find and review the meaningful change.

On the topic of the PR.
I think this change is opposed to #2137
Also it might be my fault and did not checked with enough care those cli switches, but did not expect that for a generic endpoint like admin (and health, debug, ...) we need such distinction. It is ok that some endpoint not mounted as the underlying feature not present (lightpush, filter,...).
So I think in general we can have /admin by default if having --rest=true.

@jm-clius , @fryorcraken : WDYT?

[chaitanyaprem]

I have some generic thoughts on this PR.
One littel, and yes it is a general notice to myself also as me also running into this time-to-time. It is better avoid general formatting changes when having a fix PR. Maybe you have some auto-formatting enabled, idk. But it makes harder to find and review the meaningful change.

On the topic of the PR.
I think this change is opposed to #2137
Also it might be my fault and did not checked with enough care those cli switches, but did not expect that for a generic endpoint like admin (and health, debug, ...) we need such distinction. It is ok that some endpoint not mounted as the underlying feature not present (lightpush, filter,...).
So I think in general we can have /admin by default if having --rest=true.

@jm-clius , @fryorcraken : WDYT?

Sorry, did not realise my IDE did all changes.
Let me revert and only push the 2 lines change I actually did.

[NagyZoltanPeter]

I have some generic thoughts on this PR.
One littel, and yes it is a general notice to myself also as me also running into this time-to-time. It is better avoid general formatting changes when having a fix PR. Maybe you have some auto-formatting enabled, idk. But it makes harder to find and review the meaningful change.
On the topic of the PR.
I think this change is opposed to #2137
Also it might be my fault and did not checked with enough care those cli switches, but did not expect that for a generic endpoint like admin (and health, debug, ...) we need such distinction. It is ok that some endpoint not mounted as the underlying feature not present (lightpush, filter,...).
So I think in general we can have /admin by default if having --rest=true.
@jm-clius , @fryorcraken : WDYT?

Sorry, did not realise my IDE did all changes. Let me revert and only push the 2 lines change I actually did.
Yeah I thought that was... :-)
You don't need I think. I found the relevant changes.

[fryorcraken]

I have no opinion on the matter as we are yet to understand how node operator would (or would not) use the REST API.
If anything, I'd say default to false to avoid one exposing the admin API by mistake

[Ivansete-status]

Thanks for submitting this @chaitanyaprem !
Just asking for changes to reduce its scope and avoid the automatic IDE changes.

From my point of view I think is fine to have the admin enabled by default. Although I understand this PR is aimed to allow disabling it if needed.

Thanks again, and I'll re-check once the PR is simplified.
Cheers

[Ivansete-status]

LGTM! Thanks so much indeed!

[gabrielmer]

LGTM, thank you!