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

fix(host): differentiate no config and config error #1913

Merged
merged 2 commits into from
Apr 17, 2024
Merged

Conversation

brooksmtownsend
Copy link
Member

Feature or Problem

This PR updates the handling of the get_config control interface function to return a None when a configuration isn't found, instead of an error making it difficult to tell if the configuration was absent or if there was a legitimate error.

Related Issues

Fixes #1906

Release Information

Consumer Impact

Testing

Unit Test(s)

Acceptance or Integration

Manual Verification

Signed-off-by: Brooks Townsend <brooksmtownsend@gmail.com>
Copy link
Contributor

@thomastaylor312 thomastaylor312 left a comment

Choose a reason for hiding this comment

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

This code is good to go, but we should also include an update to the get_config function in the ctl client

@brooksmtownsend
Copy link
Member Author

@thomastaylor312 I did test with the current wash and it looks like it works well, for context the code:

    let config_response = ctl_client
        .get_config(name)
        .await
        .map_err(suggest_run_host_error)?;

    sp.finish_and_clear();

    if !config_response.message.is_empty() && !config_response.success {
        error!("Error getting configuration: {}", config_response.message);
    };

    if let Some(config) = config_response.response {
        Ok(CommandOutput::new(
            format!("{:?}", config),
            config.into_iter().map(|(k, v)| (k, json!(v))).collect(),
        ))
    } else {
        Err(anyhow::anyhow!("No configuration found for name: {}", name))
    }

Perhaps we could update the doc comment to better indicate return values?

@thomastaylor312
Copy link
Contributor

Yeah, figured it worked fine, but was just more concerned about the API. I think a comment would be fine

Signed-off-by: Brooks Townsend <brooksmtownsend@gmail.com>
@brooksmtownsend brooksmtownsend merged commit 7d0a977 into main Apr 17, 2024
52 checks passed
@brooksmtownsend brooksmtownsend deleted the fix/1906 branch April 17, 2024 15:36
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.

[FEATURE] Return None instead of an error for missing configuration
2 participants