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: enable rest plugin only if in config or if --rest-http-port is set #1593

Merged
merged 3 commits into from
Dec 6, 2024

Conversation

gabrik
Copy link
Contributor

@gabrik gabrik commented Nov 13, 2024

Currently the rest plugin is enabled by default for zenohd, which means that its a required dependency when running a simple zenoh router without configuration.

This PR changes the behaviour by enabling the rest plugin only if the --rest-http-port is passed.

… passed

Signed-off-by: Gabriele Baldoni <gabriele.baldoni@gmail.com>
Copy link

PR missing one of the required labels: {'bug', 'breaking-change', 'enhancement', 'internal', 'documentation', 'dependencies', 'new feature'}

@gabrik gabrik added breaking-change Indicates that the issue implies a breaking change (be it at compile time or at runtime) internal Changes not included in the changelog labels Nov 13, 2024
@gabrik gabrik self-assigned this Nov 13, 2024
@OlivierHecart
Copy link
Contributor

OlivierHecart commented Nov 13, 2024

Should be:

    // apply '--rest-http-port' to config only if explicitly set (overwriting config)
    // or if no config file is set (to apply its default value)
    if args.rest_http_port.is_some() || args.config.is_none() {
        match args.rest_http_port.as_deref() {
            Some(value) => {
                if !value.eq_ignore_ascii_case("none") {
                    config
                        .insert_json5("plugins/rest/http_port", &format!(r#""{value}""#))
                        .unwrap();
                    config
                        .insert_json5("plugins/rest/__required__", "true")
                        .unwrap();
                }
            }
            None => {
                config
                    .insert_json5("plugins/rest/http_port", "8000")
                    .unwrap();
            }
        }
    }

@gabrik
Copy link
Contributor Author

gabrik commented Nov 13, 2024

shouldn't the || args.config.is_none() be removed to load only if the argument passed?

@OlivierHecart
Copy link
Contributor

shouldn't the || args.config.is_none() be removed to load only if the argument passed?

Well the point is that we want rest to start on port 8000 by default if nothing is specified.

@gabrik
Copy link
Contributor Author

gabrik commented Nov 13, 2024

shouldn't the || args.config.is_none() be removed to load only if the argument passed?

Well the point is that we want rest to start on port 8000 by default if nothing is specified.

the point of my PR is not not have it unless --rest-http-port is specified

@OlivierHecart
Copy link
Contributor

shouldn't the || args.config.is_none() be removed to load only if the argument passed?

Well the point is that we want rest to start on port 8000 by default if nothing is specified.

the point of my PR is not not have it unless --rest-http-port is specified

I believed the point of the PR was to not fail if rest was not available but still start it on port 8000 by default if available.

@Mallets
Copy link
Member

Mallets commented Nov 22, 2024

I agree with this PR and not having unnecessary services running if not specified.
Moreover, with current status it's impossible to compile and run zenohd without forcing the compilation of the rest plugin, which is quite annoying tbh.
The PR should also include updates to the README to explicitly set the --http-port parameter when needed.

@Mallets
Copy link
Member

Mallets commented Dec 5, 2024

Update:
Dockerfile to be updated: https://github.com/eclipse-zenoh/ci/blob/7cbe301bce9b932eea7ffea2b463890ab712f97e/publish-crates-docker/Dockerfile#L14
Current Dockerfile already passes all CLI arguments to the inner zenohd. So no new changes are expected.

@gabrik this PR is complete from PoV. Anything else?

@gabrik
Copy link
Contributor Author

gabrik commented Dec 5, 2024

Update: Dockerfile to be updated: https://github.com/eclipse-zenoh/ci/blob/7cbe301bce9b932eea7ffea2b463890ab712f97e/publish-crates-docker/Dockerfile#L14 Current Dockerfile already passes all CLI arguments to the inner zenohd. So no new changes are expected.

@gabrik this PR is complete from PoV. Anything else?

Looks good to me, but I cannot approve as I commited here.

@J-Loudet J-Loudet merged commit 65a0c04 into main Dec 6, 2024
24 checks passed
@Mallets Mallets deleted the chore/disable-rest-plugin-if-no-cli-argument branch December 6, 2024 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Indicates that the issue implies a breaking change (be it at compile time or at runtime) internal Changes not included in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants