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

Disallow booleans in environment #2000

Merged
merged 3 commits into from
Sep 15, 2015
Merged
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
11 changes: 10 additions & 1 deletion compose/config/fields_schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,16 @@

"environment": {
Copy link

Choose a reason for hiding this comment

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

should this use the list_or_dict schema? maybe there are differences between them?

Copy link
Author

Choose a reason for hiding this comment

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

I didn't use list_or_dict for this one as we define some extra constraints in the patternProperties for the allowed key names. I was being restrictive cos I was worried about character compatibility for different systems.

Copy link

Choose a reason for hiding this comment

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

I think these constraints here are the same ones we'd want for labels, and most likely for extra_hosts as well (where are the two places that list_or_dict is used).

What do you think about moving this to be the new definition of list_or_dict ?

If we do that, I think we'd want to change the format to something like non_bool_scalar

Copy link
Author

Choose a reason for hiding this comment

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

I honestly can't remember now why I thought they had separate requirements and I can't find it in my notes. If you can't think of any issues with doing that, I think that's a good suggestion, I will amend. Thanks.

Copy link

Choose a reason for hiding this comment

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

Oh, hmm, so I guess dashes are valid in label and hostnames, so that is one issue. We could just relax this to "any character" which would make it usable for all three.

Copy link
Author

Choose a reason for hiding this comment

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

I think that would be better in a separate PR as it effects other fields. I'll look at doing that after this one has gone in.

Copy link

Choose a reason for hiding this comment

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

cool, that sounds fair

"oneOf": [
{"type": "object"},
{
"type": "object",
"patternProperties": {
"^[^-]+$": {
"type": ["string", "number", "boolean"],
"format": "environment"
}
},
"additionalProperties": false
},
{"type": "array", "items": {"type": "string"}, "uniqueItems": true}
]
},
Expand Down
29 changes: 25 additions & 4 deletions compose/config/validation.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import json
import logging
import os
from functools import wraps

Expand All @@ -11,6 +12,9 @@
from .errors import ConfigurationError


log = logging.getLogger(__name__)


DOCKER_CONFIG_HINTS = {
'cpu_share': 'cpu_shares',
'add_host': 'extra_hosts',
Expand Down Expand Up @@ -44,6 +48,21 @@ def format_ports(instance):
return True


@FormatChecker.cls_checks(format="environment")
def format_boolean_in_environment(instance):
"""
Check if there is a boolean in the environment and display a warning.
Always return True here so the validation won't raise an error.
"""
if isinstance(instance, bool):
log.warn(
"Warning: There is a boolean value, {0} in the 'environment' key.\n"
"Environment variables can only be strings.\nPlease add quotes to any boolean values to make them string "
"(eg, '{0}').\nThis warning will become an error in a future release. \r\n".format(instance)
)
return True


def validate_service_names(func):
@wraps(func)
def func_wrapper(config):
Expand Down Expand Up @@ -259,23 +278,25 @@ def _parse_oneof_validator(error):

def validate_against_fields_schema(config):
schema_filename = "fields_schema.json"
return _validate_against_schema(config, schema_filename)
format_checkers = ["ports", "environment"]
return _validate_against_schema(config, schema_filename, format_checkers)


def validate_against_service_schema(config, service_name):
schema_filename = "service_schema.json"
return _validate_against_schema(config, schema_filename, service_name)
format_checkers = ["ports"]
Copy link

Choose a reason for hiding this comment

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

Why is only the ports used here, but both are used above?

Would we get double warnings if we use the environment format here?

Copy link
Author

Choose a reason for hiding this comment

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

Exactly, it gave double warnings. Currently the code path doesn't get to validate the full service schema unless it's gone through the fields validation, so thought that was the best place for it.

Copy link

Choose a reason for hiding this comment

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

Cool, maybe an inline comment for that so we remember why these lists are different?

Copy link

Choose a reason for hiding this comment

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

Thinking about this some more, I think there might be a better way to handle this. Instead of repeating the validation in the second schema, why not do something like this:

  1. validate all the fields against fields_schema.json
  2. remove the $ref: fields_schema.json#/definitions/service from service_schema.json and only validate the required fields exist. The other fields can be handled by additionalProperties: true, since we've already validated that all the fields are correct.

I think that way the format warning only logs once.

I'd actually be in favor of dropping support for abstract base services, but that's a topic for another time.

Copy link
Author

Choose a reason for hiding this comment

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

I feel like this is outside scope of this PR. Can we come back to that later in a separate PR as I think depending on how some of the future extend/import work is going to happen, this indeed will need to be re-factored but I'd like space for us to have a separate discussion around that.

Heh, yeh abstract base services are a bit of a pain.

return _validate_against_schema(config, schema_filename, format_checkers, service_name)


def _validate_against_schema(config, schema_filename, service_name=None):
def _validate_against_schema(config, schema_filename, format_checker=[], service_name=None):
config_source_dir = os.path.dirname(os.path.abspath(__file__))
schema_file = os.path.join(config_source_dir, schema_filename)

with open(schema_file, "r") as schema_fh:
schema = json.load(schema_fh)

resolver = RefResolver('file://' + config_source_dir + '/', schema)
validation_output = Draft4Validator(schema, resolver=resolver, format_checker=FormatChecker(["ports"]))
validation_output = Draft4Validator(schema, resolver=resolver, format_checker=FormatChecker(format_checker))

errors = [error for error in sorted(validation_output.iter_errors(config), key=str)]
if errors:
Expand Down
6 changes: 5 additions & 1 deletion docs/yml.md
Original file line number Diff line number Diff line change
Expand Up @@ -184,17 +184,21 @@ Mount all of the volumes from another service or container.

### environment

Add environment variables. You can use either an array or a dictionary.
Add environment variables. You can use either an array or a dictionary. Any
boolean values; true, false, yes no, need to be enclosed in quotes to ensure
they are not converted to True or False by the YML parser.

Environment variables with only a key are resolved to their values on the
machine Compose is running on, which can be helpful for secret or host-specific values.

environment:
RACK_ENV: development
SHOW: 'true'
SESSION_SECRET:

environment:
- RACK_ENV=development
- SHOW=true
- SESSION_SECRET

### env_file
Expand Down
23 changes: 20 additions & 3 deletions tests/unit/config_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -270,15 +270,32 @@ def test_valid_config_oneof_string_or_list(self):
)
self.assertEqual(service[0]['entrypoint'], entrypoint)

def test_validation_message_for_invalid_type_when_multiple_types_allowed(self):
expected_error_msg = "Service 'web' configuration key 'mem_limit' contains an invalid type, it should be a number or a string"
@mock.patch('compose.config.validation.log')
def test_logs_warning_for_boolean_in_environment(self, mock_logging):
expected_warning_msg = "Warning: There is a boolean value, True in the 'environment' key."
config.load(
config.ConfigDetails(
{'web': {
'image': 'busybox',
'environment': {'SHOW_STUFF': True}
}},
'working_dir',
'filename.yml'
)
)

self.assertTrue(mock_logging.warn.called)
self.assertTrue(expected_warning_msg in mock_logging.warn.call_args[0][0])

def test_config_invalid_environment_dict_key_raises_validation_error(self):
expected_error_msg = "Service 'web' configuration key 'environment' contains an invalid type"

with self.assertRaisesRegexp(ConfigurationError, expected_error_msg):
config.load(
config.ConfigDetails(
{'web': {
'image': 'busybox',
'mem_limit': ['incorrect']
'environment': {'---': 'nope'}
}},
'working_dir',
'filename.yml'
Expand Down