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

Valueless keys #12

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 10 additions & 14 deletions src/v2/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ pub struct Build {

/// Build arguments.
#[serde(default, skip_serializing_if = "BTreeMap::is_empty",
deserialize_with = "deserialize_map_or_key_value_list")]
pub args: BTreeMap<String, RawOr<String>>,
deserialize_with = "deserialize_map_or_key_optional_value_list")]
pub args: BTreeMap<String, Option<RawOr<String>>>,

/// PRIVATE. Mark this struct as having unknown fields for future
/// compatibility. This prevents direct construction and exhaustive
Expand Down Expand Up @@ -98,7 +98,7 @@ dockerfile: Dockerfile
let build: Build = serde_yaml::from_str(yaml).unwrap();
assert_eq!(build.context, value(Context::new(".")));
assert_eq!(build.dockerfile, Some(value("Dockerfile".to_owned())));
assert_eq!(build.args.get("key").expect("wanted key 'key'").value().unwrap(),
assert_eq!(build.args.get("key").expect("wanted key 'key'").as_ref().unwrap().value().unwrap(),
"value");
}

Expand All @@ -115,16 +115,16 @@ args:
let build: Build = serde_yaml::from_str(yaml).unwrap();

// Check type conversion.
assert_eq!(build.args.get("bool").expect("wanted key 'bool'").value().unwrap(),
assert_eq!(build.args.get("bool").expect("wanted key 'bool'").as_ref().unwrap().value().unwrap(),
"true");
assert_eq!(build.args.get("float").expect("wanted key 'float'").value().unwrap(),
assert_eq!(build.args.get("float").expect("wanted key 'float'").as_ref().unwrap().value().unwrap(),
"1.5");
assert_eq!(build.args.get("int").expect("wanted key 'int'").value().unwrap(),
assert_eq!(build.args.get("int").expect("wanted key 'int'").as_ref().unwrap().value().unwrap(),
"1");

// Check interpolation.
let mut interp: RawOr<String> =
build.args.get("interp").expect("wanted key 'interp'").to_owned();
build.args.get("interp").expect("wanted key 'interp'").as_ref().unwrap().to_owned();
env::set_var("FOO", "foo");
let env = OsEnvironment::new();
assert_eq!(interp.interpolate_env(&env).unwrap(), "foo")
Expand All @@ -136,14 +136,10 @@ fn build_args_may_be_a_key_value_list() {
context: .
args:
- key=value
- other_key
";
let build: Build = serde_yaml::from_str(yaml).unwrap();
assert_eq!(build.args.get("key").expect("should have key").value().unwrap(),
assert_eq!(build.args.get("key").expect("should have key").as_ref().unwrap().value().unwrap(),
"value");
assert_eq!(*build.args.get("other_key").expect("should have other_key"), None);
Copy link
Owner

Choose a reason for hiding this comment

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

OK, good, this is the main test that we need.

}

// TODO MED: Implement valueless keys.
//
// args:
// - buildno
// - password
23 changes: 14 additions & 9 deletions src/v2/env_file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,13 @@ use super::interpolation::{escape, RawOr};
/// A file pointed to by an `env_file:` field.
pub struct EnvFile {
/// The variables found in our env file.
vars: BTreeMap<String, String>,
vars: BTreeMap<String, Option<String>>,
}

impl EnvFile {
/// Read an `EnvFile` from a stream.
pub fn read<R: io::Read>(input: R) -> Result<EnvFile> {
let mut vars: BTreeMap<String, String> = BTreeMap::new();
let mut vars: BTreeMap<String, Option<String>> = BTreeMap::new();
let reader = io::BufReader::new(input);
for line_result in reader.lines() {
let line = line_result.chain_err(|| "I/O error")?;
Expand All @@ -28,7 +28,7 @@ impl EnvFile {
Regex::new(r#"^\s*(:?#.*)?$"#).unwrap();
// We allow lowercase env vars even if POSIX doesn't.
static ref VAR: Regex =
Regex::new(r#"^([_A-Za-z][_A-Za-z0-9]*)=(.*)"#).unwrap();
Regex::new(r#"^([_A-Za-z][_A-Za-z0-9]*)(=(.*))?"#).unwrap();
}

if BLANK.is_match(&line) {
Expand All @@ -39,7 +39,7 @@ impl EnvFile {
.ok_or_else(|| ErrorKind::ParseEnv(line.clone()))?;
vars.insert(
caps.get(1).unwrap().as_str().to_owned(),
caps.get(2).unwrap().as_str().to_owned(),
caps.get(3).map(|v| v.as_str().to_owned()),
);
}
Ok(EnvFile { vars: vars })
Expand All @@ -54,10 +54,13 @@ impl EnvFile {

/// Convert this `EnvFile` to the format we use for the `environment`
/// member of `Service`.
pub fn to_environment(&self) -> Result<BTreeMap<String, RawOr<String>>> {
pub fn to_environment(&self) -> Result<BTreeMap<String, Option<RawOr<String>>>> {
let mut env = BTreeMap::new();
for (k, v) in &self.vars {
env.insert(k.to_owned(), escape(v)?);
env.insert(k.to_owned(), match v.as_ref().map(|v| escape(v)) {
None => None,
Some(v) => Some(v?),
});
}
Ok(env)
}
Expand All @@ -79,6 +82,7 @@ fn parses_docker_compatible_env_files() {
# These are environment variables:
FOO=foo
BAR=2
BAZ
Copy link
Owner

Choose a reason for hiding this comment

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

As far as I can tell from the documentation, this is a non-standard extension to the .env format:

Compose expects each line in an env file to be in VAR=VAL format. Lines beginning with # (i.e. comments) are ignored, as are blank lines.

So unless we can find some evidence that Docker supports this extension, we should remove this code.


# Docker does not currently do anything special with quotes!
WEIRD="quoted"
Expand All @@ -88,7 +92,8 @@ WEIRD="quoted"
let cursor = io::Cursor::new(input);
let env_file = EnvFile::read(cursor).unwrap();
let env = env_file.to_environment().unwrap();
assert_eq!(env.get("FOO").unwrap().value().unwrap(), "foo");
assert_eq!(env.get("BAR").unwrap().value().unwrap(), "2");
assert_eq!(env.get("WEIRD").unwrap().value().unwrap(), "\"quoted\"");
assert_eq!(env.get("FOO").unwrap().as_ref().unwrap().value().unwrap(), "foo");
assert_eq!(env.get("BAR").unwrap().as_ref().unwrap().value().unwrap(), "2");
assert_eq!(*env.get("BAZ").unwrap(), None);
assert_eq!(env.get("WEIRD").unwrap().as_ref().unwrap().value().unwrap(), "\"quoted\"");
}
82 changes: 79 additions & 3 deletions src/v2/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ impl<'de> Deserialize<'de> for ConvertToString {
///
/// ```text
/// struct Example {
/// #[serde(deserialize_with = "deserialize_hash_or_key_value_list")]
/// #[serde(deserialize_with = "deserialize_map_or_key_value_list")]
Copy link
Owner

Choose a reason for hiding this comment

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

Good catch!

/// pub args: BTreeMap<String, RawOr<String>>,
/// }
/// ```
Expand Down Expand Up @@ -165,7 +165,83 @@ where
}
}

deserializer.deserialize_map(MapOrKeyValueListVisitor)
deserializer.deserialize_any(MapOrKeyValueListVisitor)
}

/// Like `deserialize_map_or_key_value_list`, but allowing missing values
/// (e.g. for environment variables)
pub fn deserialize_map_or_key_optional_value_list<'de, D>(
deserializer: D,
) -> Result<BTreeMap<String, Option<RawOr<String>>>, D::Error>
where
D: Deserializer<'de>,
{
/// Declare an internal visitor type to handle our input.
struct MapOrKeyOptionalValueListVisitor;

impl<'de> Visitor<'de> for MapOrKeyOptionalValueListVisitor {
type Value = BTreeMap<String, Option<RawOr<String>>>;

// We have a real map.
fn visit_map<V>(self, mut visitor: V) -> Result<Self::Value, V::Error>
where
V: MapAccess<'de>,
{
let mut map: BTreeMap<String, Option<RawOr<String>>> = BTreeMap::new();
while let Some(key) = visitor.next_key::<String>()? {
if map.contains_key(&key) {
let msg = format!("duplicate map key: {}", &key);
return Err(<V::Error as de::Error>::custom(msg));
}
let ConvertToString(val) = visitor.next_value::<ConvertToString>()?;
let raw_or_value = raw(val)
.map_err(|e| <V::Error as de::Error>::custom(format!("{}", e)))?;
map.insert(key, Some(raw_or_value));
}
Ok(map)
}

// We have a key/value list. Yuck.
fn visit_seq<V>(self, mut visitor: V) -> Result<Self::Value, V::Error>
where
V: SeqAccess<'de>,
{
lazy_static! {
// Match a key/value pair.
static ref KEY_VALUE: Regex =
Regex::new("^([^=]+)(=(.*))?$").unwrap();
}

let mut map: BTreeMap<String, Option<RawOr<String>>> = BTreeMap::new();
while let Some(key_value) = visitor.next_element::<String>()? {
let caps = KEY_VALUE.captures(&key_value).ok_or_else(|| {
let msg = format!("expected KEY[=value], got: <{}>", &key_value);
<V::Error as de::Error>::custom(msg)
})?;
let key = caps.get(1).unwrap().as_str();
let value = caps.get(3).map(|v| v.as_str());
if map.contains_key(key) {
let msg = format!("duplicate map key: {}", key);
return Err(<V::Error as de::Error>::custom(msg));
}
let optional_raw_or_value = match value.map(|value| {
raw(value.to_owned())
.map_err(|e| <V::Error as de::Error>::custom(format!("{}", e)))
}) {
None => None,
Some(x) => Some(x?),
};
map.insert(key.to_owned(), optional_raw_or_value);
}
Ok(map)
}

fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
write!(formatter, "a map or a key/[value] list")
}
}

deserializer.deserialize_any(MapOrKeyOptionalValueListVisitor)
}

/// Given a map, deserialize it normally. But if we have a list of string
Expand Down Expand Up @@ -211,7 +287,7 @@ where
}
}

deserializer.deserialize_map(MapOrDefaultListVisitor(PhantomData::<T>))
deserializer.deserialize_any(MapOrDefaultListVisitor(PhantomData::<T>))
}

/// Deserialize either list or a single bare string as a list.
Expand Down
4 changes: 2 additions & 2 deletions src/v2/network.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@ pub struct Network {
/// Docker labels for this volume, specifying various sorts of
/// custom metadata.
#[serde(default, skip_serializing_if = "BTreeMap::is_empty",
deserialize_with = "deserialize_map_or_key_value_list")]
pub labels: BTreeMap<String, RawOr<String>>,
deserialize_with = "deserialize_map_or_key_optional_value_list")]
pub labels: BTreeMap<String, Option<RawOr<String>>>,

// TODO LOW: ipam

Expand Down
8 changes: 4 additions & 4 deletions src/v2/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ pub struct Service {

/// Environment variables and values to supply to the container.
#[serde(default, skip_serializing_if = "BTreeMap::is_empty",
deserialize_with = "deserialize_map_or_key_value_list")]
pub environment: BTreeMap<String, RawOr<String>>,
deserialize_with = "deserialize_map_or_key_optional_value_list")]
Copy link
Owner

Choose a reason for hiding this comment

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

Have we looked at all the other callers of deserialize_map_or_key_value_list and verified that none of them require similar conversions? I can check this if you want, but it will probably have to wait until sometime next week at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested all of them with docker_compose and the only one that doesn't accept valueless keys is driver_opts. All the others do.

This raises some mildly interesting questions in cage, such as what to do if io.fdy.cage.srcdir is set but null.

pub environment: BTreeMap<String, Option<RawOr<String>>>,

/// Expose a list of ports to any containers that link to us.
#[serde(default, skip_serializing_if = "Vec::is_empty")]
Expand All @@ -98,8 +98,8 @@ pub struct Service {
/// Docker labels for this container, specifying various sorts of
/// custom metadata.
#[serde(default, skip_serializing_if = "BTreeMap::is_empty",
deserialize_with = "deserialize_map_or_key_value_list")]
pub labels: BTreeMap<String, RawOr<String>>,
deserialize_with = "deserialize_map_or_key_optional_value_list")]
pub labels: BTreeMap<String, Option<RawOr<String>>>,

/// Links to other services in this file.
#[serde(default, skip_serializing_if = "Vec::is_empty")]
Expand Down
4 changes: 2 additions & 2 deletions src/v2/volume.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ pub struct Volume {
/// Docker labels for this volume, specifying various sorts of
/// custom metadata.
#[serde(default, skip_serializing_if = "BTreeMap::is_empty",
deserialize_with = "deserialize_map_or_key_value_list")]
pub labels: BTreeMap<String, RawOr<String>>,
deserialize_with = "deserialize_map_or_key_optional_value_list")]
pub labels: BTreeMap<String, Option<RawOr<String>>>,

/// PRIVATE. Mark this struct as having unknown fields for future
/// compatibility. This prevents direct construction and exhaustive
Expand Down