ronantakizawa/codereview-bench huggingface dataset:
Repository: bottlerocket-os/bottlerocket
File: sources/api/pluto/src/hyper_proxy/mod.rs (Rust)

Code before:
            uri,
            headers: HeaderMap::new(),
            force_connect: false,
        }
    }
}

/// A wrapper around `Proxy`s with a connector.
#[derive(Clone)]
pub struct ProxyConnector<C> {
    proxies: Vec<Proxy>,
    connector: C,
    tls: Option<TlsConnector>,
}

impl<C: fmt::Debug> fmt::Debug for ProxyConnector<C> {
    fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
        write!(
            f,
            "ProxyConnector {}{{ proxies: {:?}, connector: {:?} }}",
            if self.tls.is_some() {
                ""
            } else {
                "(unsecured)"
            },
            self.proxies,
            self.connector
        )
    }
}

impl<C> ProxyConnector<C> {
    /// Create a new secured Proxies
    pub fn new(connector: C) -> Result<Self, io::Error> {
        let config = ClientConfig::builder()
            .with_safe_defaults()
            .with_native_roots()
            .with_no_client_auth();

        let cfg = Arc::new(config);
        let tls = TlsConnector::from(cfg);

        Ok(ProxyConnector {
            proxies: Vec::new(),
            connector,
            tls: Some(tls),
        })
    }

    /// Create a proxy connector and attach a particular proxy
    pub fn from_proxy(connector: C, proxy: Proxy) -> Result<Self, io::Error> {

Review comment (security):
Why do we need to set the authorization headers in the `_` (specifically, `None`) case where connections aren't going through the proxy?

Code after:
}

impl Proxy {
    /// Create a new `Proxy`
    pub fn new<I: Into<Intercept>>(intercept: I, uri: Uri) -> Proxy {
        Proxy {
            intercept: intercept.into(),
            uri,
            headers: HeaderMap::new(),
            force_connect: false,
        }
    }

    /// Set `Proxy` authorization
    pub fn set_authorization<C: Credentials + Clone>(&mut self, credentials: Authorization<C>) {
        match self.intercept {
            Intercept::Https => {
                self.headers.typed_insert(ProxyAuthorization(credentials.0));
            }
            _ => {
                self.headers
                    .typed_insert(Authorization(credentials.0.clone()));
                self.headers.typed_insert(ProxyAuthorization(credentials.0));
            }
        }
    }
}

/// A wrapper around `Proxy`s with a connector.
#[derive(Clone)]
pub struct ProxyConnector<C> {
    proxies: Vec<Proxy>,
    connector: C,
    tls: Option<TlsConnector>,
}

impl<C: fmt::Debug> fmt::Debug for ProxyConnector<C> {
    fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> {
        write!(
            f,
            "ProxyConnector {}{{ proxies: {:?}, connector: {:?} }}",
            if self.tls.is_some() {
                ""
            } else {
                "(unsecured)"
            },
            self.proxies,
            self.connector
        )
    }
}
Repository: bottlerocket-os/bottlerocket
File: sources/api/migration/migrations/v1.19.2/certdog-config-file-v0-1-0/src/main.rs (Rust)

Code before:
use migration_helpers::common_migrations::AddPrefixesMigration;
use migration_helpers::{migrate, Result};
use std::process;

/// Add settings for the new certdog-toml config file
fn run() -> Result<()> {
    migrate(AddPrefixesMigration(vec![
        "services.pki",
        "configuration-files.certdog-toml",
    ]))
}

fn main() {
    if let Err(e) = run() {
        eprintln!("{}", e);
        process::exit(1);
    }
}


Review comment (bug):
This isn't correct - `services.pki` was added a long time ago (in v1.2.0).

`AddPrefixesMigration` could also be written as `RemovePrefixesOnDowngradeMigration` - it's a no-op on upgrade but does the equivalent of `rm -rf` on downgrade. If used incorrectly then previously valid settings may go missing.

Code after:
use migration_helpers::common_migrations::AddPrefixesMigration;
use migration_helpers::{migrate, Result};
use std::process;

/// Add settings for the new certdog-toml config file
fn run() -> Result<()> {
    migrate(AddPrefixesMigration(vec![
        "configuration-files.certdog-toml",
    ]))
}

fn main() {
    if let Err(e) = run() {
        eprintln!("{}", e);
        process::exit(1);
    }
}
Repository: bottlerocket-os/bottlerocket
File: sources/api/bootstrap-containers/src/main.rs (Rust)

Code before:
/// Stores the subcommand to be executed
#[derive(Debug)]
enum Subcommand {
    CreateContainers,
    MarkBootstrap(MarkBootstrapArgs),
}

#[derive(Debug)]
struct MarkBootstrapArgs {
    container_id: String,
    mode: BootstrapContainerMode,
}

/// Print a usage message in the event a bad arg is passed
fn usage() {
    let program_name = env::args().next().unwrap_or_else(|| "program".to_string());
    eprintln!(
        r"Usage: {} SUBCOMMAND [ ARGUMENTS... ]

    Subcommands:
        create-containers
        mark-bootstrap

    Global arguments:
        [ --config-path PATH ]
        [ --socket-path PATH ]
        [ --log-level trace|debug|info|warn|error ]

    Mark bootstrap arguments:
        --container-id CONTAINER-ID
        --mode MODE

    Config path defaults to {}
    Socket path defaults to {}",
        program_name,
        DEFAULT_CONFIG_PATH,
        constants::API_SOCKET
    );
}

/// Parses user arguments into an Args struct
fn parse_args(args: env::Args) -> Result<(Args, Subcommand)> {
    let mut global_args = Args::default();
    let mut subcommand = None;
    let mut subcommand_args = Vec::new();

    let mut iter = args.skip(1);
    while let Some(arg) = iter.next() {
        match arg.as_ref() {
            // Global args
            "--log-level" => {

Review comment (suggestion):
Is the --socket-path still required? If not we should either 

a) note that it is deprecated and give a warning if the command line arg is used

or, preferably

b) remove the argument from the cli

Edit: Oh I see it still used. Probably to mount it into the container it is starting.

Code after:
    config_path: PathBuf,
}

impl Default for Args {
    fn default() -> Self {
        Self {
            log_level: LevelFilter::Info,
            config_path: PathBuf::from_str(DEFAULT_CONFIG_PATH).unwrap(),
        }
    }
}

/// Stores the subcommand to be executed
#[derive(Debug)]
enum Subcommand {
    CreateContainers,
    MarkBootstrap(MarkBootstrapArgs),
}

#[derive(Debug)]
struct MarkBootstrapArgs {
    container_id: String,
    mode: BootstrapContainerMode,
}

/// Print a usage message in the event a bad arg is passed
fn usage() {
    let program_name = env::args().next().unwrap_or_else(|| "program".to_string());
    eprintln!(
        r"Usage: {} SUBCOMMAND [ ARGUMENTS... ]

    Subcommands:
        create-containers
        mark-bootstrap

    Global arguments:
        [ --config-path PATH ]
        [ --log-level trace|debug|info|warn|error ]

    Mark bootstrap arguments:
        --container-id CONTAINER-ID
        --mode MODE

    Config path defaults to {}",
        program_name, DEFAULT_CONFIG_PATH,
    );
}

/// Parses user arguments into an Args struct
fn parse_args(args: env::Args) -> Result<(Args, Subcommand)> {
    let mut global_args = Args::default();
Repository: bottlerocket-os/bottlerocket
File: sources/api/bootstrap-containers/src/main.rs (Rust)

Code before:
/// Stores user-supplied global arguments
#[derive(Debug)]
struct Args {
    log_level: LevelFilter,
    config_path: PathBuf,
    socket_path: String,
}

impl Default for Args {
    fn default() -> Self {
        Self {
            log_level: LevelFilter::Info,
            config_path: PathBuf::from_str(DEFAULT_CONFIG_PATH).unwrap(),
            socket_path: constants::API_SOCKET.to_string(),
        }
    }
}

/// Stores the subcommand to be executed
#[derive(Debug)]
enum Subcommand {
    CreateContainers,
    MarkBootstrap(MarkBootstrapArgs),
}

#[derive(Debug)]
struct MarkBootstrapArgs {
    container_id: String,
    mode: BootstrapContainerMode,
}

/// Print a usage message in the event a bad arg is passed
fn usage() {
    let program_name = env::args().next().unwrap_or_else(|| "program".to_string());
    eprintln!(
        r"Usage: {} SUBCOMMAND [ ARGUMENTS... ]

    Subcommands:
        create-containers
        mark-bootstrap

    Global arguments:
        [ --config-path PATH ]
        [ --socket-path PATH ]
        [ --log-level trace|debug|info|warn|error ]

    Mark bootstrap arguments:
        --container-id CONTAINER-ID
        --mode MODE

    Config path defaults to {}

Review comment (refactor):
We need to clean up the other use of `socket_path` in `mark_bootstrap` - the larger goal of this series of changes is to remove the datastore dependency from these programs entirely.

It's going to be a bit more involved. What I'd suggest trying is calling `apiclient set bootstrap-containers.%i=off` instead. Not sure that will work but maybe. 

Code after:

#[derive(Serialize, Deserialize)]
#[serde(rename_all = "kebab-case")]
struct BootstrapContainer {
    #[serde(default, skip_serializing_if = "Option::is_none")]
    source: Option<Url>,
    #[serde(default, skip_serializing_if = "Option::is_none")]
    mode: Option<BootstrapContainerMode>,
    #[serde(default, skip_serializing_if = "Option::is_none")]
    user_data: Option<ValidBase64>,
    #[serde(default, skip_serializing_if = "Option::is_none")]
    essential: Option<bool>,
}

/// Stores user-supplied global arguments
#[derive(Debug)]
struct Args {
    log_level: LevelFilter,
    config_path: PathBuf,
}

impl Default for Args {
    fn default() -> Self {
        Self {
            log_level: LevelFilter::Info,
            config_path: PathBuf::from_str(DEFAULT_CONFIG_PATH).unwrap(),
        }
    }
}

/// Stores the subcommand to be executed
#[derive(Debug)]
enum Subcommand {
    CreateContainers,
    MarkBootstrap(MarkBootstrapArgs),
}

#[derive(Debug)]
struct MarkBootstrapArgs {
    container_id: String,
    mode: BootstrapContainerMode,
}

/// Print a usage message in the event a bad arg is passed
fn usage() {
    let program_name = env::args().next().unwrap_or_else(|| "program".to_string());
    eprintln!(
        r"Usage: {} SUBCOMMAND [ ARGUMENTS... ]

    Subcommands:
        create-containers
Repository: bottlerocket-os/bottlerocket
File: sources/host-ctr/cmd/host-ctr/main.go (Go)

Code before:
		// For all other registries
		return defaultResolver
	}
}

// withUnmaskedPaths sets an alternate list of masked paths, less the paths provided
func withUnmaskedPaths(unmaskPaths []string) oci.SpecOpts {
	return func(_ context.Context, _ oci.Client, _ *containers.Container, s *runtimespec.Spec) error {
		if s.Linux != nil && s.Linux.MaskedPaths != nil {
			var maskedPaths []string
			for _, path := range s.Linux.MaskedPaths {
				if SliceContains(unmaskPaths, path) {
					continue
				}
				maskedPaths = append(maskedPaths, path)
			}
			// Replace the default Linux.MaskedPaths with our own
			s.Linux.MaskedPaths = maskedPaths
		}
		return nil
	}
}

// Convert labels to map[string]string for containerd.WithPullLabels.
// Labels are in the format of "key=value,key2=value2".
func convertLabels(labels string) map[string]string {
	labelsMap := make(map[string]string)
	labelsList := strings.Split(labels, ",")
	for _, label := range labelsList {
		labelKeyValue := strings.Split(label, "=")
		labelsMap[labelKeyValue[0]] = labelKeyValue[1]
	}
	return labelsMap
}


Review comment (suggestion):
In general, input conversion and validation should happen as close to where data enters the system as possible, so that you're not passing around potentially invalid or loosely typed data.

Code after:

		return func(_ *containerd.Client, c *containerd.RemoteContext) error {
			resolver := docker.NewResolver(resolverOpt)
			log.G(ctx).WithField("ref", ref).Info("pulling from ECR Public")
			c.Resolver = resolver
			return nil
		}
	default:
		// For all other registries
		return defaultResolver
	}
}

// withUnmaskedPaths sets an alternate list of masked paths, less the paths provided
func withUnmaskedPaths(unmaskPaths []string) oci.SpecOpts {
	return func(_ context.Context, _ oci.Client, _ *containers.Container, s *runtimespec.Spec) error {
		if s.Linux != nil && s.Linux.MaskedPaths != nil {
			var maskedPaths []string
			for _, path := range s.Linux.MaskedPaths {
				if SliceContains(unmaskPaths, path) {
					continue
				}
				maskedPaths = append(maskedPaths, path)
			}
			// Replace the default Linux.MaskedPaths with our own
			s.Linux.MaskedPaths = maskedPaths
		}
		return nil
	}
}

// Convert label to map[string]string for containerd.WithPullLabels.
// Label are in the format of "key=value".
func convertLabels(labels []string) (map[string]string, error) {
	labelsMap := make(map[string]string)
	// a slice of labels is empty if no labels are provided. Then we should return an empty map.
	if len(labels) == 0 {
		return labelsMap, nil
	}

	for _, label := range labels {
		labelLen := len(label)
		if labelLen > imageLabelMaxSize {
			return labelsMap, fmt.Errorf("label key and value length (%d bytes) greater than maximum size (%d bytes)", labelLen, imageLabelMaxSize)
		}

		if strings.Contains(label, "=") {
			labelKeyValue := strings.Split(label, "=")
			labelsMap[labelKeyValue[0]] = labelKeyValue[1]
		} else {
			labelsMap[label] = ""
Repository: bottlerocket-os/bottlerocket
File: sources/host-ctr/cmd/host-ctr/main.go (Go)

Code before:
			s.Linux.MaskedPaths = maskedPaths
		}
		return nil
	}
}

// Convert label to map[string]string for containerd.WithPullLabels.
// Label are in the format of "key=value".
func convertLabels(labels string) (map[string]string, error) {
	labelsMap := make(map[string]string)
	labelsList := strings.Split(labels, ",")
	for _, label := range labelsList {
		labelKeyValue := strings.Split(label, "=")

		if len(labelKeyValue) != 2 {
			return labelsMap, fmt.Errorf("invalid label format: %s. It should be key=value", label)
		}
		if err := Validate(labelKeyValue[0], labelKeyValue[1]); err != nil {
			return labelsMap, err
		}

		labelsMap[labelKeyValue[0]] = labelKeyValue[1]
	}
	return labelsMap, nil
}

// Validate a label's key and value are under 4096 bytes
//
// validation logic derived from:
// https://github.com/containerd/containerd/blob/f5e7fe0cb6286333a597a407029fe7c760626669/pkg/labels/validate.go#L32-L41
func Validate(k, v string) error {
	total := len(k) + len(v)
	if total > maxSize {
		if len(k) > keyMaxLen {
			k = k[:keyMaxLen]
		}
		return fmt.Errorf("label key and value length (%d bytes) greater than maximum size (%d bytes), key: %s: %w", total, maxSize, k, errdefs.ErrInvalidArgument)
	}
	return nil
}


Review comment (suggestion):
You could also tolerate input without an `=` and just assume it's a key where the value is the empty string.

Code after:
		}
		return nil
	}
}

// Convert label to map[string]string for containerd.WithPullLabels.
// Label are in the format of "key=value".
func convertLabels(labels []string) (map[string]string, error) {
	labelsMap := make(map[string]string)
	// a slice of labels is empty if no labels are provided. Then we should return an empty map.
	if len(labels) == 0 {
		return labelsMap, nil
	}

	for _, label := range labels {
		labelLen := len(label)
		if labelLen > imageLabelMaxSize {
			return labelsMap, fmt.Errorf("label key and value length (%d bytes) greater than maximum size (%d bytes)", labelLen, imageLabelMaxSize)
		}

		if strings.Contains(label, "=") {
			labelKeyValue := strings.Split(label, "=")
			labelsMap[labelKeyValue[0]] = labelKeyValue[1]
		} else {
			labelsMap[label] = ""
		}
	}
	return labelsMap, nil
}
Repository: bottlerocket-os/bottlerocket
File: sources/api/host-containers/src/config.rs (Rust)

Code before:
use model_derive::model;
use modeled_types::{Identifier, Url, ValidBase64};
use std::collections::HashMap;

#[model]
struct HostContainersConfig {
    host_containers: HashMap<Identifier, HostContainer>,
}

#[model]
struct HostContainer {
    source: Url,
    enabled: bool,
    superpowered: bool,
    user_data: ValidBase64,
}


Review comment (suggestion):
Using  `model` makes me a little squeamish, perhaps irrationally. What is it that this macro is doing for you? Is it just turning the fields into `Options` and implementing `Deserialize`? If so, I would prefer that be explicitly done rather than hiding information from the programmer in a fairly anemic procedural macro.

I feel the same way about `modeled-types` just due to its associate the "The Model", however I see that you are using `ValidBase64` which is real/desirable code re-use.

Code after:
use modeled_types::{Identifier, Url, ValidBase64};
use serde::Deserialize;
use std::collections::HashMap;

#[derive(Deserialize, Debug, PartialEq)]
#[serde(rename_all = "kebab-case")]
pub(crate) struct HostContainersConfig {
    pub(crate) host_containers: Option<HashMap<Identifier, HostContainer>>,
}

#[derive(Deserialize, Debug, PartialEq)]
#[serde(rename_all = "kebab-case")]
pub(crate) struct HostContainer {
    pub(crate) source: Option<Url>,
    pub(crate) enabled: Option<bool>,
    pub(crate) superpowered: Option<bool>,
    pub(crate) user_data: Option<ValidBase64>,
}
Repository: bottlerocket-os/bottlerocket
File: sources/api/schnauzer/src/helpers.rs (Rust)

Code before:
    }
}

#[cfg(test)]
mod test_if_not_null {
    use super::*;
    use handlebars::RenderError;
    use serde::Serialize;
    use serde_json::json;

    // A thin wrapper around the handlebars render_template method that includes
    // setup and registration of helpers
    fn setup_and_render_template<T>(tmpl: &str, data: &T) -> Result<String, RenderError>
    where
        T: Serialize,
    {
        let mut registry = Handlebars::new();
        registry.register_helper("if-not-null", Box::new(IfNotNullHelper));

        registry.render_template(tmpl, data)
    }

    #[test]
    fn null_value() {
        let result = setup_and_render_template(
            "{{#if-not-null setting}}foo{{/if-not-null}}",
            &json!({"not-the-setting": 42.42}),
        )
        .unwrap();
        assert_eq!(result, "")
    }

    #[test]
    fn explicit_null_value() {
        let result = setup_and_render_template(
            "{{#if-not-null setting}}foo{{/if-not-null}}",
            &json!({"setting": None::<()>}),
        )
        .unwrap();
        assert_eq!(result, "")
    }

    #[test]
    fn falsy_number_value() {
        let result = setup_and_render_template(
            "{{#if-not-null setting}}foo{{/if-not-null}}",
            &json!({"setting": 0}),
        )
        .unwrap();
        assert_eq!(result, "foo")
    }

Review comment (suggestion):
Is it possible to create this so that the natural/built-in conditionals work as expected, i.e. the native `#if`?

```suggestion
            "{{#if not_null setting}}foo{{/if}}",
```

Code after:
    }
}

#[cfg(test)]
mod test_if_not_null {
    use super::*;
    use handlebars::RenderError;
    use serde::Serialize;
    use serde_json::json;

    // A thin wrapper around the handlebars render_template method that includes
    // setup and registration of helpers
    fn setup_and_render_template<T>(tmpl: &str, data: &T) -> Result<String, RenderError>
    where
        T: Serialize,
    {
        let mut registry = Handlebars::new();
        registry.register_helper("if_not_null", Box::new(IfNotNullHelper));

        registry.render_template(tmpl, data)
    }

    #[test]
    fn null_value() {
        let result =
            setup_and_render_template("{{#if_not_null setting}}foo{{/if_not_null}}", &json!({}))
                .unwrap();
        assert_eq!(result, "")
    }

    #[test]
    fn render_else() {
        let result = setup_and_render_template(
            "{{#if_not_null setting}}foo{{else}}bar{{/if_not_null}}",
            &json!({}),
        )
        .unwrap();
        assert_eq!(result, "bar")
    }

    #[test]
    fn explicit_null_value() {
        let result = setup_and_render_template(
            "{{#if_not_null setting}}foo{{/if_not_null}}",
            &json!({"setting": None::<()>}),
        )
        .unwrap();
        assert_eq!(result, "")
    }

    #[test]
Repository: bottlerocket-os/bottlerocket
File: sources/settings-extensions/ntp/src/lib.rs (Rust)

Code before:
/// The ntp settings can be used to specify time servers with which to synchronize the instance's
/// clock.
use bottlerocket_settings_sdk::{GenerateResult, LinearlyMigrateable, NoMigration, SettingsModel};
use model_derive::model;
use modeled_types::Url;
use std::convert::Infallible;

#[model(impl_default = true)]
pub struct NtpSettingsV1 {
    time_servers: Vec<Url>,
    options: Vec<str>,
}

type Result<T> = std::result::Result<T, Infallible>;

impl SettingsModel for NtpSettingsV1 {
    /// the `model` macro makes every field of the `NtpSettingsV1` struct an `Option`, so we can use
    /// the type as its own `PartialKind`.
    type PartialKind = Self;
    type ErrorKind = Infallible;

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

    fn set(_current_value: Option<Self>, _target: Self) -> Result<()> {
        // Anything that parses as a list of URLs is ok
        Ok(())
    }

    fn generate(
        existing_partial: Option<Self::PartialKind>,
        _dependent_settings: Option<serde_json::Value>,
    ) -> Result<GenerateResult<Self::PartialKind, Self>> {
        Ok(GenerateResult::Complete(
            existing_partial.unwrap_or_default(),

Review comment (suggestion):
You'll need to use an owned string here:

```suggestion
    options: Vec<String>,
```

Code after:
/// The ntp settings can be used to specify time servers with which to synchronize the instance's
/// clock.
use bottlerocket_settings_sdk::{GenerateResult, LinearlyMigrateable, NoMigration, SettingsModel};
use model_derive::model;
use modeled_types::Url;
use std::convert::Infallible;

#[model(impl_default = true)]
pub struct NtpSettingsV1 {
    time_servers: Vec<Url>,
    options: Vec<String>,
}

type Result<T> = std::result::Result<T, Infallible>;

impl SettingsModel for NtpSettingsV1 {
    /// the `model` macro makes every field of the `NtpSettingsV1` struct an `Option`, so we can use
    /// the type as its own `PartialKind`.
    type PartialKind = Self;
    type ErrorKind = Infallible;

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

    fn set(_current_value: Option<Self>, _target: Self) -> Result<()> {
        // Anything that parses as a list of URLs is ok
        Ok(())
    }

    fn generate(
        existing_partial: Option<Self::PartialKind>,
        _dependent_settings: Option<serde_json::Value>,
    ) -> Result<GenerateResult<Self::PartialKind, Self>> {
        Ok(GenerateResult::Complete(
            existing_partial.unwrap_or_default(),