Repository: mattjmcnaughton/sdep

Issue:
Add `index_suffix` and `error_key` as optional fields
When telling AWS to have our bucket perform like a website, with this [boto call](http://boto3.readthedocs.io/en/latest/reference/services/s3.html#S3.Client.put_bucket_website), we have the option of specifying which suffix will represent an index file (currently `index.html`) and the file to display when there is an error (currently `404.html`). Make both of these configurable options with sensible defaults.

Discussion:

Patch:
diff --git a/docs/cli.rst b/docs/cli.rst
index ab80b6a..62fcaee 100644
--- a/docs/cli.rst
+++ b/docs/cli.rst
@@ -61,6 +61,12 @@ options can be set in configuration files:
 
 **Optional**
 
+- :command:`INDEX_SUFFIX`: When hosting with Amazon S3, it is necessary to
+  specify an index suffix, which is appended to all urls ending in :command:`/`. The
+  default value is :command:`index.html`.
+- :command:`ERROR_KEY`: The S3 key of the file Amazon should serve in case of
+  error (i.e. incorrect url). The default value is :command:`404.html`.
+
 Environment Variables
 ~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/sdep/app.py b/sdep/app.py
index ecf518f..ad923f8 100644
--- a/sdep/app.py
+++ b/sdep/app.py
@@ -28,12 +28,6 @@ class Sdep(object):
     # Constant names of AWS objects.
     BUCKET_NAME = "bucket_name"
 
-    # Default index and error.
-    # @TODO This specification is temporary, as eventually we will make these
-    # configurable options with `Config`.
-    DEFAULT_INDEX_SUFFIX = "index.html"
-    DEFAULT_ERROR_KEY = "404.html"
-
     def __init__(self, config):
         self._config = config
         self._s3_client = self._establish_s3_client()
@@ -199,10 +193,10 @@ class Sdep(object):
         """
         website_config = {
             "IndexDocument": {
-                "Suffix": self.DEFAULT_INDEX_SUFFIX
+                "Suffix": self._config.get(Config.INDEX_SUFFIX_FIELD)
             },
             "ErrorDocument": {
-                "Key": self.DEFAULT_ERROR_KEY
+                "Key": self._config.get(Config.ERROR_KEY_FIELD)
             }
         }
 
diff --git a/sdep/config.py b/sdep/config.py
index 636f97a..22298f8 100644
--- a/sdep/config.py
+++ b/sdep/config.py
@@ -65,6 +65,8 @@ class Config(object):
     AWS_SECRET_ACCESS_KEY_FIELD = "aws_secret_access_key"
     SITE_DIR_FIELD = "site_dir"
     DOMAIN_FIELD = "domain"
+    INDEX_SUFFIX_FIELD = "index_suffix"
+    ERROR_KEY_FIELD = "error_key"
 
     def __init__(self, config_file=None, test_mode=False):
         # @TODO I wonder if it would make more sense for the `Config` class to
@@ -144,13 +146,11 @@ class Config(object):
             ConfigImproperFormatError: If vital configuration data is either in
                 the incorrect format or nonexistent.
         """
-        for field in self.required_config_fields(env=True):
-            value = os.environ.get(field)
-
-            if value is None:
-                raise ConfigImproperFormatError
-            else:
-                self._config_hash[field.lower()] = value
+        self._config_hash = self._parse_from_store(
+            self.required_config_fields(env=True),
+            self.optional_config_fields(env=True),
+            os.environ
+        )
 
     def _parse_from_config_file(self, config_file):
         """
@@ -172,15 +172,46 @@ class Config(object):
         except (IOError, json.JSONDecodeError):
             raise ConfigImproperFormatError
 
-        # @TODO Should a common helper method implement this functionality
-        # for both `_parse_from_config_file` and `_parse_from_env`.
-        for field in self.required_config_fields(env=False):
-            value = config_data.get(field)
+        self._config_hash = self._parse_from_store(
+            self.required_config_fields(env=False),
+            self.optional_config_fields(env=False),
+            config_data
+        )
+
+    @classmethod
+    def _parse_from_store(cls, required_fields, optional_fields, data_store):
+        """
+        Parse the configuration from a data store object (i.e. the json hash or
+        `os.environ`). This method is useful because the process for parsing the
+        data from either the environment or a configuration file shares many of
+        the same components. Abstracting to a single method ensures less
+        duplicate code.
+
+        Args:
+            required_fields (list): A list of the required fields.
+            optional_fields (list): A list of the optional fields.
+            data_store (dict): A dictionary containing key/value pairs with the
+                fields as a key.
+
+        Returns:
+            dict: A configuration dictionary.
+        """
+        # Start with all of the defaults filled in. We will overwrite with any
+        # specified info.
+        config_hash = cls._optional_fields_and_defaults()
+
+        fields = [(f, True) for f in required_fields] + [(f, False) for f in optional_fields]
+
+        for field, required in fields:
+            value = data_store.get(field)
 
             if value is None:
-                raise ConfigImproperFormatError
+                if required:
+                    raise ConfigImproperFormatError
             else:
-                self._config_hash[field.lower()] = value
+                config_hash[field.lower()] = value
+
+        return config_hash
 
     @classmethod
     def required_config_fields(cls, env=False):
@@ -207,8 +238,8 @@ class Config(object):
         else:
             return required_fields
 
-    @staticmethod
-    def optional_config_fields(env=False):
+    @classmethod
+    def optional_config_fields(cls, env=False):
         """
         Return the optinal configuration fields either in `snake_case` or in all
         upper-case `snake_case`, depending on whether the `env` flag is set.
@@ -220,23 +251,40 @@ class Config(object):
         Returns:
             [str]: A list of optional configuration fields.
         """
-        optional_fields = []
+        optional_fields = list(cls._optional_fields_and_defaults().keys())
 
         if env:
             return [field.upper() for field in optional_fields]
         else:
             return optional_fields
 
+    @classmethod
+    def _optional_fields_and_defaults(cls):
+        """
+        Return a dictionary of optional fields and their defaults.
+
+        Returns:
+            dict: Optional fields and their defaults.
+        """
+        return {
+            cls.INDEX_SUFFIX_FIELD: "index.html",
+            cls.ERROR_KEY_FIELD: "404.html"
+        }
+
     def _prepopulate_config(self):
         """
         Prepopulate this instance of `Config` with sensible default values which
         we can use when testing.
         """
-        # @TODO Determine a better method for automatically including all
-        # `required` variables.
-        self._config_hash = {
+        populate_hash = {
             self.AWS_ACCESS_KEY_ID_FIELD: "MY_ACCESS_KEY",
             self.AWS_SECRET_ACCESS_KEY_FIELD: "MY_SECRET_KEY",
             self.SITE_DIR_FIELD: "./static",
             self.DOMAIN_FIELD: "sdep-test.com"
         }
+
+        self._config_hash = self._parse_from_store(
+            self.required_config_fields(env=False),
+            self.optional_config_fields(env=False),
+            populate_hash
+        )


Test patch:
diff --git a/tests/test_app.py b/tests/test_app.py
index 8dd700f..08b5da0 100644
--- a/tests/test_app.py
+++ b/tests/test_app.py
@@ -117,9 +117,8 @@ class SdepTestCase(unittest.TestCase):
         # with `Config`.
         resp = self._s3_client.get_bucket_website(Bucket=bucket_name)
 
-        self.assertEqual(resp["IndexDocument"]["Suffix"],
-                         Sdep.DEFAULT_INDEX_SUFFIX)
-        self.assertEqual(resp["ErrorDocument"]["Key"], Sdep.DEFAULT_ERROR_KEY)
+        self.assertNotEqual(resp["IndexDocument"]["Suffix"], None)
+        self.assertNotEqual(resp["ErrorDocument"]["Key"], None)
 
     @classmethod
     def _create_test_upload_dir(cls):
diff --git a/tests/test_config.py b/tests/test_config.py
index 0feb7be..a1be9d8 100644
--- a/tests/test_config.py
+++ b/tests/test_config.py
@@ -35,7 +35,7 @@ class ConfigTestCase(unittest.TestCase):
         config_file = self._create_config_file()
         config = Config(config_file=config_file)
 
-        for field in Config.required_config_fields():
+        for field in self._all_fields():
             self.assertNotEqual(config.get(field), None)
 
         os.remove(config_file)
@@ -51,7 +51,7 @@ class ConfigTestCase(unittest.TestCase):
         with patch.dict(os.environ, environ_dict, clear=True):
             config = Config()
 
-            for field in Config.required_config_fields():
+            for field in self._all_fields():
                 self.assertNotEqual(config.get(field), None)
 
     def test_find_config_in_curr_dir(self):
@@ -71,7 +71,7 @@ class ConfigTestCase(unittest.TestCase):
                 config = Config()
 
                 self.assertEqual(config_in_curr, Config.locate_config_file())
-                for field in Config.required_config_fields():
+                for field in self._all_fields():
                     self.assertNotEqual(config.get(field), None)
 
         for temp_dir in [temp_dirs.current, temp_dirs.home]:
@@ -97,13 +97,12 @@ class ConfigTestCase(unittest.TestCase):
                 config = Config()
 
                 self.assertEqual(config_in_home, Config.locate_config_file())
-                for field in Config.required_config_fields():
+                for field in self._all_fields():
                     self.assertNotEqual(config.get(field), None)
 
         for temp_dir in [temp_dirs.current, temp_dirs.home]:
             shutil.rmtree(temp_dir, ignore_errors=True)
 
-
     def test_bad_config(self):
         """
         Test loading the configuration from a file with an improperly specified
@@ -114,15 +113,23 @@ class ConfigTestCase(unittest.TestCase):
         with self.assertRaises(ConfigParseError):
             Config(config_file=config_file)
 
-    @staticmethod
-    def _config_dict():
+    @classmethod
+    def _config_dict(cls):
         """
         A dictionary of property formatted config.
 
         Returns:
             dict: A properly formatted config.
         """
-        return {field: str(uuid.uuid4()) for field in Config.required_config_fields()}
+        base_dict = {field: str(uuid.uuid4()) for field in cls._all_fields()}
+
+        # Remove one of the optional fields so that we can test the default value
+        # being filled in.
+
+        field_to_remove = Config.optional_config_fields()[0]
+        del base_dict[field_to_remove]
+
+        return base_dict
 
     @classmethod
     def _create_mock_dirs(cls):
@@ -176,3 +183,13 @@ class ConfigTestCase(unittest.TestCase):
             bad_config_file.write(json.dumps({}))
 
         return file_name
+
+    @staticmethod
+    def _all_fields():
+        """
+        Helper method to return all configuration fields.
+
+        Returns:
+            list: List of the strings for all configuration fields.
+        """
+        return Config.required_config_fields() + Config.optional_config_fields()