ronantakizawa/codereview-bench huggingface dataset:
Repository: bottlerocket-os/bottlerocket
File: sources/settings-extensions/ntp/src/lib.rs (Rust)

Code before:
    fn test_serde_ntp() {
        let test_json = r#"{"time-servers":["https://example.net","http://www.example.com"]}"#;

        let ntp: NtpSettingsV1 = serde_json::from_str(test_json).unwrap();
        assert_eq!(
            ntp.time_servers.clone().unwrap(),
            vec!(
                Url::try_from("https://example.net").unwrap(),
                Url::try_from("http://www.example.com").unwrap(),
            )
        );

        let results = serde_json::to_string(&ntp).unwrap();
        assert_eq!(results, test_json);
    }

    fn test_options_ntp() {
        let test_json = r#"{"time-servers":["https://example.net","http://www.example.com"],"options": ["minpoll", "1", "maxpoll", "2"]}"#;

        let ntp: NtpSettingsV1 = serde_json::from_str(test_json).unwrap();
        assert_eq!(
            ntp.options.clone().unwrap(),
            vec!(
                Url::try_from("minpool").unwrap(),
                Url::try_from("1").unwrap(),
                Url::try_from("maxpool").unwrap(),
                Url::try_from("2").unwrap(),
            )
        );

        let results = serde_json::to_string(&ntp).unwrap();
        assert_eq!(results, test_json);
    }
}


Review comment (suggestion):
```suggestion
                Url::try_from("minpoll").unwrap(),
                Url::try_from("1").unwrap(),
                Url::try_from("maxpoll").unwrap(),
```

Code after:
    fn test_serde_ntp() {
        let test_json = r#"{"time-servers":["https://example.net","http://www.example.com"]}"#;

        let ntp: NtpSettingsV1 = serde_json::from_str(test_json).unwrap();
        assert_eq!(
            ntp.time_servers.clone().unwrap(),
            vec!(
                Url::try_from("https://example.net").unwrap(),
                Url::try_from("http://www.example.com").unwrap(),
            )
        );

        let results = serde_json::to_string(&ntp).unwrap();
        assert_eq!(results, test_json);
    }

    #[test]
    fn test_options_ntp() {
        let test_json = r#"{"time-servers":["https://example.net","http://www.example.com"],"options":["minpoll","1","maxpoll","2"]}"#;

        let ntp: NtpSettingsV1 = serde_json::from_str(test_json).unwrap();
        assert_eq!(
            ntp.options.clone().unwrap(),
            vec!("minpoll", "1", "maxpoll", "2",)
        );

        let results = serde_json::to_string(&ntp).unwrap();
        assert_eq!(results, test_json);
    }
}
Repository: bottlerocket-os/bottlerocket
File: sources/settings-extensions/ntp/src/lib.rs (Rust)

Code before:
            Ok(GenerateResult::Complete(NtpSettingsV1 {
                time_servers: None
                options: None
            }))
        )
    }

    #[test]
    fn test_serde_ntp() {
        let test_json = r#"{"time-servers":["https://example.net","http://www.example.com"]}"#;

        let ntp: NtpSettingsV1 = serde_json::from_str(test_json).unwrap();
        assert_eq!(
            ntp.time_servers.clone().unwrap(),
            vec!(
                Url::try_from("https://example.net").unwrap(),
                Url::try_from("http://www.example.com").unwrap(),
            )
        );

        let results = serde_json::to_string(&ntp).unwrap();
        assert_eq!(results, test_json);
    }

    fn test_options_ntp() {
        let test_json = r#"{"time-servers":["https://example.net","http://www.example.com"],"options": ["minpoll", "1", "maxpoll", "2"]}"#;

        let ntp: NtpSettingsV1 = serde_json::from_str(test_json).unwrap();
        assert_eq!(
            ntp.options.clone().unwrap(),
            vec!(
                Url::try_from("minpoll").unwrap(),
                Url::try_from("1").unwrap(),
                Url::try_from("maxpoll").unwrap(),
                Url::try_from("2").unwrap(),
            )
        );

        let results = serde_json::to_string(&ntp).unwrap();
        assert_eq!(results, test_json);
    }
}


Review comment (suggestion):
For testing `serde_json::to_string()` at the end of this test, you'll need to remove all of the extra whitespace from this string:

```suggestion
        let test_json = r#"{"time-servers":["https://example.net","http://www.example.com"],"options":["minpoll","1","maxpoll","2"]}"#;
```

Code after:
            Ok(GenerateResult::Complete(NtpSettingsV1 {
                time_servers: None,
                options: None,
            }))
        )
    }

    #[test]
    fn test_serde_ntp() {
        let test_json = r#"{"time-servers":["https://example.net","http://www.example.com"]}"#;

        let ntp: NtpSettingsV1 = serde_json::from_str(test_json).unwrap();
        assert_eq!(
            ntp.time_servers.clone().unwrap(),
            vec!(
                Url::try_from("https://example.net").unwrap(),
                Url::try_from("http://www.example.com").unwrap(),
            )
        );

        let results = serde_json::to_string(&ntp).unwrap();
        assert_eq!(results, test_json);
    }

    #[test]
    fn test_options_ntp() {
        let test_json = r#"{"time-servers":["https://example.net","http://www.example.com"],"options":["minpoll","1","maxpoll","2"]}"#;

        let ntp: NtpSettingsV1 = serde_json::from_str(test_json).unwrap();
        assert_eq!(
            ntp.options.clone().unwrap(),
            vec!("minpoll", "1", "maxpoll", "2",)
        );

        let results = serde_json::to_string(&ntp).unwrap();
        assert_eq!(results, test_json);
    }
}
Repository: bottlerocket-os/bottlerocket
File: sources/settings-extensions/ntp/src/lib.rs (Rust)

Code before:
            Ok(GenerateResult::Complete(NtpSettingsV1 {
                time_servers: None
                options: None
            }))
        )
    }

    #[test]
    fn test_serde_ntp() {
        let test_json = r#"{"time-servers":["https://example.net","http://www.example.com"]}"#;

        let ntp: NtpSettingsV1 = serde_json::from_str(test_json).unwrap();
        assert_eq!(
            ntp.time_servers.clone().unwrap(),
            vec!(
                Url::try_from("https://example.net").unwrap(),
                Url::try_from("http://www.example.com").unwrap(),
            )
        );

        let results = serde_json::to_string(&ntp).unwrap();
        assert_eq!(results, test_json);
    }

    fn test_options_ntp() {
        let test_json = r#"{"time-servers":["https://example.net","http://www.example.com"],"options": ["minpoll", "1", "maxpoll", "2"]}"#;

        let ntp: NtpSettingsV1 = serde_json::from_str(test_json).unwrap();
        assert_eq!(
            ntp.options.clone().unwrap(),
            vec!(
                Url::try_from("minpoll").unwrap(),
                Url::try_from("1").unwrap(),
                Url::try_from("maxpoll").unwrap(),
                Url::try_from("2").unwrap(),
            )
        );

        let results = serde_json::to_string(&ntp).unwrap();
        assert_eq!(results, test_json);
    }
}


Review comment (suggestion):
In order for Rust to recognize this as a unit test, you need to provide the `#[test]` macro here:

```suggestion
    #[test]
    fn test_options_ntp() {
```

Code after:
            Ok(GenerateResult::Complete(NtpSettingsV1 {
                time_servers: None,
                options: None,
            }))
        )
    }

    #[test]
    fn test_serde_ntp() {
        let test_json = r#"{"time-servers":["https://example.net","http://www.example.com"]}"#;

        let ntp: NtpSettingsV1 = serde_json::from_str(test_json).unwrap();
        assert_eq!(
            ntp.time_servers.clone().unwrap(),
            vec!(
                Url::try_from("https://example.net").unwrap(),
                Url::try_from("http://www.example.com").unwrap(),
            )
        );

        let results = serde_json::to_string(&ntp).unwrap();
        assert_eq!(results, test_json);
    }

    #[test]
    fn test_options_ntp() {
        let test_json = r#"{"time-servers":["https://example.net","http://www.example.com"],"options":["minpoll","1","maxpoll","2"]}"#;

        let ntp: NtpSettingsV1 = serde_json::from_str(test_json).unwrap();
        assert_eq!(
            ntp.options.clone().unwrap(),
            vec!("minpoll", "1", "maxpoll", "2",)
        );

        let results = serde_json::to_string(&ntp).unwrap();
        assert_eq!(results, test_json);
    }
}
Repository: bottlerocket-os/bottlerocket
File: sources/settings-extensions/ntp/src/lib.rs (Rust)

Code before:
        let test_json = r#"{"time-servers":["https://example.net","http://www.example.com"]}"#;

        let ntp: NtpSettingsV1 = serde_json::from_str(test_json).unwrap();
        assert_eq!(
            ntp.time_servers.clone().unwrap(),
            vec!(
                Url::try_from("https://example.net").unwrap(),
                Url::try_from("http://www.example.com").unwrap(),
            )
        );

        let results = serde_json::to_string(&ntp).unwrap();
        assert_eq!(results, test_json);
    }

    fn test_options_ntp() {
        let test_json = r#"{"time-servers":["https://example.net","http://www.example.com"],"options": ["minpoll", "1", "maxpoll", "2"]}"#;

        let ntp: NtpSettingsV1 = serde_json::from_str(test_json).unwrap();
        assert_eq!(
            ntp.options.clone().unwrap(),
            vec!(
                Url::try_from("minpoll").unwrap(),
                Url::try_from("1").unwrap(),
                Url::try_from("maxpoll").unwrap(),
                Url::try_from("2").unwrap(),
            )
        );

        let results = serde_json::to_string(&ntp).unwrap();
        assert_eq!(results, test_json);
    }
}


Review comment (suggestion):
These should be `String`s, not `Url`s:

```suggestion
                String::from("minpoll"),
                String::from("1"),
                String::from("maxpoll"),
                String::from("2"),
```

Code after:
        let test_json = r#"{"time-servers":["https://example.net","http://www.example.com"]}"#;

        let ntp: NtpSettingsV1 = serde_json::from_str(test_json).unwrap();
        assert_eq!(
            ntp.time_servers.clone().unwrap(),
            vec!(
                Url::try_from("https://example.net").unwrap(),
                Url::try_from("http://www.example.com").unwrap(),
            )
        );

        let results = serde_json::to_string(&ntp).unwrap();
        assert_eq!(results, test_json);
    }

    #[test]
    fn test_options_ntp() {
        let test_json = r#"{"time-servers":["https://example.net","http://www.example.com"],"options":["minpoll","1","maxpoll","2"]}"#;

        let ntp: NtpSettingsV1 = serde_json::from_str(test_json).unwrap();
        assert_eq!(
            ntp.options.clone().unwrap(),
            vec!("minpoll", "1", "maxpoll", "2",)
        );

        let results = serde_json::to_string(&ntp).unwrap();
        assert_eq!(results, test_json);
    }
}
Repository: bottlerocket-os/bottlerocket
File: sources/dogtag/bin/imds.rs (Rust)

Code before:

Currently the following hostname tools are implemented:

* 01-imds - Fetches hostname from the Instance Metadata via IMDS
* 00-reverse-dns - Uses reverse dns lookup to resolve the hostname
 */
use dogtag::Cli;
use imdsclient::ImdsClient;
use snafu::{OptionExt, ResultExt};

type Result<T> = std::result::Result<T, error::Error>;

/// Implements a hostname lookup tool by fetching the public hostname
/// from the instance metadata via IMDS. It will interface with IMDS
/// via:
///
/// * Check for IPv6, default to IPv4 if not available
/// * Check for IMDSv2, fallback to IMDSv1 if not enabled
#[tokio::main]
async fn main() -> Result<()> {
    // Even though for this helper we do not need any arguments
    // still validate to ensure the helper follows standards.
    let _: Cli = argh::from_env();
    let mut imds = ImdsClient::new();
    let hostname = imds.fetch_hostname().await.context(error::ImdsSnafu)?;
    println!("{}", hostname.context(error::NoHostnameSnafu)?);
    Ok(())
}

mod error {
    use snafu::Snafu;

    #[derive(Debug, Snafu)]
    #[snafu(visibility(pub(super)))]
    pub(super) enum Error {
        #[snafu(display("failed to fetch hostname from IMDS: {}", source))]
        Imds { source: imdsclient::Error },
        #[snafu(display("no hostname returned by imds"))]
        NoHostname,
    }
}


Review comment (suggestion):
nit: I think I'd prefer we handle all the errors right away, e.g.:

```suggestion
    let hostname = imds.fetch_hostname().await
            .context(error::ImdsSnafu)?
            .context(error::NoHostnameSnafu)?;
    println!("{}", hostname);
```

Code after:

type Result<T> = std::result::Result<T, error::Error>;

/// Implements a hostname lookup tool by fetching the public hostname
/// from the instance metadata via IMDS. It will interface with IMDS
/// via:
///
/// * Check for IPv6, default to IPv4 if not available
/// * Check for IMDSv2, fallback to IMDSv1 if not enabled
#[tokio::main]
async fn main() -> Result<()> {
    // Even though for this helper we do not need any arguments
    // still validate to ensure the helper follows standards.
    let _: Cli = argh::from_env();
    let mut imds = ImdsClient::new();
    let hostname = imds
        .fetch_hostname()
        .await
        .context(error::ImdsSnafu)?
        .context(error::NoHostnameSnafu)?;
    println!("{}", hostname);
    Ok(())
}

mod error {
    use snafu::Snafu;

    #[derive(Debug, Snafu)]
    #[snafu(visibility(pub(super)))]
    pub(super) enum Error {
        #[snafu(display("failed to fetch hostname from IMDS: {}", source))]
        Imds { source: imdsclient::Error },
        #[snafu(display("no hostname returned by imds"))]
        NoHostname,
    }
}
Repository: bottlerocket-os/bottlerocket
File: sources/models/src/aws-k8s-1.30-nvidia/mod.rs (Rust)

Code before:
use crate::{
    AutoScalingSettings, BootSettings, BootstrapContainer, CloudFormationSettings,
    ContainerRuntimeSettings, DnsSettings, HostContainer, KubernetesSettings,
    NetworkSettings, OciDefaults, OciHooks, PemCertificate,
};
use modeled_types::Identifier;

use model_derive::model;
use std::collections::HashMap;

// Note: we have to use 'rename' here because the top-level Settings structure is the only one
// that uses its name in serialization; internal structures use the field name that points to it
#[model(rename = "settings", impl_default = true)]
struct Settings {
    motd: settings_extension_motd::MotdV1,
    kubernetes: KubernetesSettings,
    updates: settings_extension_updates::UpdatesSettingsV1,
    host_containers: HashMap<Identifier, HostContainer>,
    bootstrap_containers: HashMap<Identifier, BootstrapContainer>,
    ntp: settings_extension_ntp::NtpSettingsV1,
    network: NetworkSettings,
    kernel: settings_extension_kernel::KernelSettingsV1,
    boot: BootSettings,
    aws: settings_extension_aws::AwsSettingsV1,
    metrics: settings_extension_metrics::MetricsSettingsV1,
    pki: HashMap<Identifier, PemCertificate>,
    container_registry: settings_extension_container_registry::RegistrySettingsV1,
    oci_defaults: OciDefaults,

Review comment (suggestion):
You'll want to run `rustfmt` on all of these files, that will automatically sort the imports. Here I think that `NetworkSettings` should be on line 3

Code after:
use crate::{
    AutoScalingSettings, BootSettings, BootstrapContainer, CloudFormationSettings,
    ContainerRuntimeSettings, DnsSettings, HostContainer, KubernetesSettings, NetworkSettings,
    OciDefaults, OciHooks,
};
use modeled_types::Identifier;

use model_derive::model;
use std::collections::HashMap;

// Note: we have to use 'rename' here because the top-level Settings structure is the only one
// that uses its name in serialization; internal structures use the field name that points to it
#[model(rename = "settings", impl_default = true)]
struct Settings {
    motd: settings_extension_motd::MotdV1,
    kubernetes: KubernetesSettings,
    updates: settings_extension_updates::UpdatesSettingsV1,
    host_containers: HashMap<Identifier, HostContainer>,
    bootstrap_containers: HashMap<Identifier, BootstrapContainer>,
    ntp: settings_extension_ntp::NtpSettingsV1,
    network: NetworkSettings,
    kernel: settings_extension_kernel::KernelSettingsV1,
    boot: BootSettings,
    aws: settings_extension_aws::AwsSettingsV1,
    metrics: settings_extension_metrics::MetricsSettingsV1,
    pki: settings_extension_pki::PkiSettingsV1,
    container_registry: settings_extension_container_registry::RegistrySettingsV1,
    oci_defaults: OciDefaults,
Repository: bottlerocket-os/bottlerocket
File: sources/settings-extensions/pki/src/lib.rs (Rust)

Code before:

    fn get_version() -> &'static str {
        "v1"
    }

    fn set(_current_value: Option<Self>, _target: Self) -> Result<()> {
        // Set anything that parses as PkiSettingsV1.
        Ok(())
    }

    fn generate(
        _existing_partial: Option<Self::PartialKind>,
        _dependent_settings: Option<serde_json::Value>,
    ) -> Result<GenerateResult<Self::PartialKind, Self>> {
        Ok(GenerateResult::Complete(PkiSettingsV1 {
            pki: HashMap::new(),
        }))
    }

    fn validate(_value: Self, _validated_settings: Option<serde_json::Value>) -> Result<()> {
        // Validate anything that parses as PkiSettingsV1.
        Ok(())
    }
}

impl LinearlyMigrateable for PkiSettingsV1 {
    type ForwardMigrationTarget = NoMigration;
    type BackwardMigrationTarget = NoMigration;

    fn migrate_forward(&self) -> Result<Self::ForwardMigrationTarget> {
        NoMigration::no_defined_migration()
    }

    fn migrate_backward(&self) -> Result<Self::BackwardMigrationTarget> {
        NoMigration::no_defined_migration()
    }
}

#[cfg(test)]
mod test {
    use super::*;

    static VALID_PEM: &str = include_str!("../tests/data/test-pem");

    #[test]
    fn test_generate_pki_settings() {
        assert_eq!(
            PkiSettingsV1::generate(None, None),
            Ok(GenerateResult::Complete(PkiSettingsV1 {
                pki: HashMap::new(),
            }))

Review comment (suggestion):
this trait is not needed for the `NullMigrator`, it's only used to provide an interface to the `LinearMigrator`. if we later need migrations for this setting, we can implement this, but for now let's remove it.

Code after:

    fn get_version() -> &'static str {
        "v1"
    }

    fn set(_current_value: Option<Self>, _target: Self) -> Result<()> {
        // Set anything that parses as PkiSettingsV1.
        Ok(())
    }

    fn generate(
        _existing_partial: Option<Self::PartialKind>,
        _dependent_settings: Option<serde_json::Value>,
    ) -> Result<GenerateResult<Self::PartialKind, Self>> {
        Ok(GenerateResult::Complete(PkiSettingsV1 {
            pki: HashMap::new(),
        }))
    }

    fn validate(_value: Self, _validated_settings: Option<serde_json::Value>) -> Result<()> {
        // Validate anything that parses as PkiSettingsV1.
        Ok(())
    }
}

#[cfg(test)]
mod test {
    use super::*;
    use serde_json::json;

    static VALID_PEM: &str = include_str!("../tests/data/test-pem");

    #[test]
    fn test_generate_pki_settings() {
        assert_eq!(
            PkiSettingsV1::generate(None, None),
            Ok(GenerateResult::Complete(PkiSettingsV1 {
                pki: HashMap::new(),
            }))
        )
    }

    #[test]
    fn test_serde_pki() {
        let test_json = json!({
            "foo": {
                "data": VALID_PEM,
                "trusted": true
            }
        });
Repository: bottlerocket-os/bottlerocket
File: sources/settings-extensions/pki/src/lib.rs (Rust)

Code before:
    }

    #[test]
    fn test_serde_pki() {
        let test_json = format!(
            r#"{{
            "foo": {{
                "data": "{}",
                "trusted": true
            }}
        }}"#,
            VALID_PEM
        );

        let pki: PkiSettingsV1 = serde_json::from_str(&test_json).unwrap();

        let mut expected_pki: HashMap<Identifier, PemCertificate> = HashMap::new();
        expected_pki.insert(
            Identifier::try_from("foo").unwrap(),
            PemCertificate {
                data: Some(PemCertificateString::try_from(VALID_PEM).unwrap()),
                trusted: Some(true),
            },
        );

        assert_eq!(pki, PkiSettingsV1 { pki: expected_pki });
    }
}


Review comment (suggestion):
I'm realizing that many of the other settings extensions don't have this, but we should test the serialize path as well, e.g https://github.com/bottlerocket-os/bottlerocket/blob/develop/sources/settings-extensions/ntp/src/lib.rs#L87-L88

Code after:
        )
    }

    #[test]
    fn test_serde_pki() {
        let test_json = json!({
            "foo": {
                "data": VALID_PEM,
                "trusted": true
            }
        });

        let test_json_str = test_json.to_string();

        let pki: PkiSettingsV1 = serde_json::from_str(&test_json_str).unwrap();

        let mut expected_pki: HashMap<Identifier, PemCertificate> = HashMap::new();
        expected_pki.insert(
            Identifier::try_from("foo").unwrap(),
            PemCertificate {
                data: Some(PemCertificateString::try_from(VALID_PEM).unwrap()),
                trusted: Some(true),
            },
        );

        assert_eq!(pki, PkiSettingsV1 { pki: expected_pki });

        let serialized_json: serde_json::Value = serde_json::to_string(&pki)
            .map(|s| serde_json::from_str(&s).unwrap())
            .unwrap();

        assert_eq!(serialized_json, test_json);
    }
}