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

Add support for experimental features #1053

Merged
merged 7 commits into from
Jan 30, 2019
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
5 changes: 5 additions & 0 deletions chalice/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -688,6 +688,10 @@ def __init__(self, app_name, debug=False, configure_logs=True, env=None):
if env is None:
env = os.environ
self._initialize(env)
self.experimental_feature_flags = set()
# This is marked as internal but is intended to be used by
# any code within Chalice.
self._features_used = set()
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I'm not sure if I like this trend of marking something as an internal property and then accessing it in the deployer code. I think we did something similar in blueprints. I get why we are doing it right now, but I'm wondering if there are any alternatives to tell users that it is an internal property? Options that I am thinking include: Just documenting it as internal (somewhat similar to some of the standard library) or having like an property like app.internal_stuff that could be a grab bag for anything that needs to be accessed in the deployer but should not be used in a user's Chalice app.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we've learned that lesson from botocore that this isn't a great idea. It basically comes down to either:

a) We make it public, call it something that's hopefully obvious (e.g. internal_stuff) and document that users shouldn't touch it and hope that they don't.

b) Make it actually internal with a _ prefix, and then we (chalice devs) purposefully access internal attributes we know are safe.

I think (b) is a much safer alternative, especially given that the _ prefix is a much more explicit mechanism. We need the concept of "package private" and I would prefer the option that requires chalice devs to be mindful of what they're accessing (e.g that _features_used is safe) vs. customers knowing that they shouldn't access an attribute called internal_features_used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with james. I think the convention for a friend-ish class should just be marked private, its a lot safer for everyone involved.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess its fine. The only thing I do not like about the _ prefix is that it can be hard to know which _ properties are public for our development only. The only way to really tell is to look for the comment in the code where it is defined. We also have to be diligent in making sure we test the _ property so we do not accidentally refactor it in a backwards incompatible way (but I think we have a good amount of tests for the ones we have added so that should be fine).


def _initialize(self, env):
if self.configure_logs:
Expand Down Expand Up @@ -735,6 +739,7 @@ def _configure_log_level(self):
self.log.setLevel(level)

def register_blueprint(self, blueprint, name_prefix=None, url_prefix=None):
self._features_used.add('BLUEPRINTS')
blueprint.register(self, options={'name_prefix': name_prefix,
'url_prefix': url_prefix})

Expand Down
3 changes: 3 additions & 0 deletions chalice/app.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,9 @@ class Chalice(DecoratorAPI):
builtin_auth_handlers = ... # type: List[BuiltinAuthConfig]
event_sources = ... # type: List[BaseEventSourceConfig]
pure_lambda_functions = ... # type: List[LambdaFunction]
# Used for feature flag validation
_features_used = ... # type: Set[str]
experimental_feature_flags = ... # type: Set[str]

def __init__(self, app_name: str, debug: bool=False,
configure_logs: bool=True,
Expand Down
4 changes: 4 additions & 0 deletions chalice/cli/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
from chalice.logs import display_logs
from chalice.utils import create_zip_file
from chalice.deploy.validate import validate_routes, validate_python_version
from chalice.deploy.validate import ExperimentalFeatureError
from chalice.utils import getting_started_prompt, UI, serialize_to_json
from chalice.constants import CONFIG_VERSION, TEMPLATE_APP, GITIGNORE
from chalice.constants import DEFAULT_STAGE_NAME
Expand Down Expand Up @@ -466,6 +467,9 @@ def main():
"environment variable or set the "
"region value in our ~/.aws/config file.", err=True)
return 2
except ExperimentalFeatureError as e:
click.echo(str(e))
return 2
except Exception:
click.echo(traceback.format_exc(), err=True)
return 2
10 changes: 8 additions & 2 deletions chalice/cli/factory.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
from chalice.utils import UI # noqa
from chalice.utils import PipeReader # noqa
from chalice.deploy import deployer # noqa
from chalice.deploy import validate
from chalice.invoke import LambdaInvokeHandler
from chalice.invoke import LambdaInvoker
from chalice.invoke import LambdaResponseFormatter
Expand Down Expand Up @@ -222,8 +223,11 @@ def create_lambda_invoke_handler(self, name, stage):

return handler

def load_chalice_app(self, environment_variables=None):
# type: (Optional[MutableMapping]) -> Chalice
def load_chalice_app(self, environment_variables=None,
validate_feature_flags=True):
# type: (Optional[MutableMapping], Optional[bool]) -> Chalice
# validate_features indicates that we should validate that
# any expiremental features used have the appropriate feature flags.
if self.project_dir not in sys.path:
sys.path.insert(0, self.project_dir)
# The vendor directory has its contents copied up to the top level of
Expand Down Expand Up @@ -258,6 +262,8 @@ def load_chalice_app(self, environment_variables=None):
'SyntaxError: %s'
) % (getattr(e, 'filename'), e.lineno, e.text, e.msg)
raise RuntimeError(message)
if validate_feature_flags:
validate.validate_feature_flags(chalice_app)
return chalice_app

def load_project_config(self):
Expand Down
12 changes: 12 additions & 0 deletions chalice/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,18 @@ def index():
"""


EXPERIMENTAL_ERROR_MSG = """

You are using experimental features without explicitly opting in.
Experimental features do not guarantee backwards compatibility and may be
removed in the future. If you'd still like to use these experimental features,
you can opt in by adding this to your app.py file:\n\n%s

See https://chalice.readthedocs.io/en/latest/topics/experimental.html for more
details.
"""


SQS_EVENT_SOURCE_POLICY = {
"Effect": "Allow",
"Action": [
Expand Down
30 changes: 30 additions & 0 deletions chalice/deploy/validate.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,24 @@

from chalice import app # noqa
from chalice.config import Config # noqa
from chalice.constants import EXPERIMENTAL_ERROR_MSG


class ExperimentalFeatureError(Exception):
def __init__(self, features_missing_opt_in):
# type: (Set[str]) -> None
self.features_missing_opt_in = features_missing_opt_in
msg = self._generate_msg(features_missing_opt_in)
super(ExperimentalFeatureError, self).__init__(msg)

def _generate_msg(self, missing_features):
# type: (Set[str]) -> str
opt_in_line = (
'app.experimental_feature_flags.update([\n'
'%s\n'
'])\n' % ',\n'.join([" '%s'" % feature
for feature in missing_features]))
return EXPERIMENTAL_ERROR_MSG % opt_in_line


def validate_configuration(config):
Expand All @@ -23,6 +41,18 @@ def validate_configuration(config):
_validate_manage_iam_role(config)
validate_python_version(config)
validate_unique_function_names(config)
validate_feature_flags(config.chalice_app)


def validate_feature_flags(chalice_app):
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to have some sort of warning/alert that something has been removed from the feature flag list and is now a stable feature? Looks like it doesn't really matter but if I upgrade chalice and redeploy and blueprints was moved to stable. I will still have my BLUEPRINTS entry in the experimental feature flags list when not necessary.

Copy link
Member

Choose a reason for hiding this comment

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

+1, though it doesn't necessarily need to be added now since we won't be using it yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd prefer not to. I think warnings should only be for things that may cause issues for a user (i.e. a deprecation of something). In this case it's just unnecessary code that runs.

Perhaps in the future we can have a some sort of linting command that can check these things, but I don't think it needs to happen at deploy time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Linting command seems good. Agreed it doesn't need to be deploy time.

# type: (app.Chalice) -> None
missing_opt_in = set()
# pylint: disable=protected-access
for feature in chalice_app._features_used:
if feature not in chalice_app.experimental_feature_flags:
missing_opt_in.add(feature)
if missing_opt_in:
raise ExperimentalFeatureError(missing_opt_in)


def validate_routes(routes):
Expand Down
1 change: 1 addition & 0 deletions docs/source/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ Topics
topics/purelambda
topics/blueprints
topics/cd
topics/experimental


API Reference
Expand Down
36 changes: 26 additions & 10 deletions docs/source/topics/blueprints.rst
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,26 @@ Blueprints
==========


.. warning::

Blueprints are considered an experimental API. You'll need to opt-in
to this feature using the ``BLUEPRINTS`` feature flag:

.. code-block:: python

app = Chalice('myapp')
app.experimental_feature_flags.extend([
'BLUEPRINTS'
])

See :doc:`experimental` for more information.


Chalice blueprints are used to organize your application into logical
components. Using a blueprint, you define your resources and decorators in
modules outside of your ``app.py``. You then register a blueprint in your main
``app.py`` file. Any decorator supported on an application object is also
supported in a blueprint.
``app.py`` file. Blueprints support any decorator available on an application
object.


.. note::
Expand All @@ -15,14 +30,14 @@ supported in a blueprint.
<http://flask.pocoo.org/docs/latest/blueprints/>`__ in Flask. Flask
blueprints allow you to define a set of URL routes separately from the main
``Flask`` object. This concept is extended to all resources in Chalice. A
Chalice blueprint can have Lambda functions, event handlers, built in
Chalice blueprint can have Lambda functions, event handlers, built-in
authorizers, etc. in addition to a collection of routes.


Example
-------

In this example we'll create a blueprint with part of our routes defined in a
In this example, we'll create a blueprint with part of our routes defined in a
separate file. First, let's create an application::

$ chalice new-project blueprint-demo
Expand All @@ -46,10 +61,11 @@ Next, we'll oen the ``chalicelib/blueprints.py`` file:
return {'foo': 'bar'}


The ``__name__`` is used to denote the import path of the blueprint. This must
match the import name of the module so the function can be properly imported
when running in Lambda. We'll now import this module in our ``app.py`` and
register this blueprint. We'll also add a route in our ``app.py`` directly:
The ``__name__`` is used to denote the import path of the blueprint. This name
must match the import name of the module so the function can be properly
imported when running in Lambda. We'll now import this module in our
``app.py`` and register this blueprint. We'll also add a route in our
``app.py`` directly:

.. code-block:: python

Expand All @@ -64,7 +80,7 @@ register this blueprint. We'll also add a route in our ``app.py`` directly:
def index():
return {'hello': 'world'}

At this point we've defined two routes. One route, ``/``, is directly defined
At this point, we've defined two routes. One route, ``/``, is directly defined
in our ``app.py`` file. The other route, ``/foo`` is defined in
``chalicelib/blueprints.py``. It was added to our Chalice app when we
registered it via ``app.register_blueprint(extra_routes)``.
Expand Down Expand Up @@ -124,7 +140,7 @@ Blueprint Registration
The ``app.register_blueprint`` function accepts two optional arguments,
``name_prefix`` and ``url_prefix``. This allows you to register the resources
in your blueprint at a certain url and name prefix. If you specify
``url_prefix`` any routes defined in your blueprint will have the
``url_prefix``, any routes defined in your blueprint will have the
``url_prefix`` prepended to it. If you specify the ``name_prefix``, any Lambda
functions created will have the ``name_prefix`` prepended to the resource name.

Expand Down
90 changes: 90 additions & 0 deletions docs/source/topics/experimental.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
Experimental APIs
=================

Chalice maintains backwards compatibility for all features that appear in this
documentation. Any Chalice application using version 1.x will continue to work
for all future versions of 1.x.

We also believe that Chalice has a lot of potential for new ideas and APIs,
many of which will take several iterations to get right. We may implement a
new idea and need to make changes based on customer usage and feedback. This
may include backwards incompatible changes all the way up to the removal of
a feature.

To accommodate these new features, Chalice has support for experimental APIs,
which are features that are added to Chalice on a provisional basis. Because
these features may include backwards incompatible changes, you must explicitly
opt-in to using these features. This makes it clear that you are using an
experimental feature that may change.

Opting-in to Experimental APIs
------------------------------

Each experimental feature in chalice has a name associated with it. To opt-in
to an experimental API, you must have the feature name to the
``experimental_feature_flags`` attribute on your ``app`` object.
This attribute's type is a set of strings.

.. code-block:: python

from chalice import Chalice

app = Chalice('myapp')
app.experimental_feature_flags.update([
'MYFEATURE1',
'MYFEATURE2',
])


If you use an experimental API without opting-in, you will receive
a message whenever you run a Chalice CLI command. The error message
tells you which feature flags you need to add::

$ chalice deploy
You are using experimental features without explicitly opting in.
Experimental features do not guarantee backwards compatibility and may be removed in the future.
If you still like to use these experimental features, you can opt-in by adding this to your app.py file:
Copy link
Member

Choose a reason for hiding this comment

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

If you would


app.experimental_feature_flags.update([
'BLUEPRINTS'
])


See https://chalice.readthedocs.io/en/latest/topics/experimental.rst for more details.

The feature flag only happens when running CLI commands. There are no runtime
checks for experimental features once your application is deployed.


List of Experimental APIs
-------------------------

In the table below, the "Feature Flag Name" column is the value you
must add to the ``app.experimental_feature_flags`` attribute.
The status of an experimental API can be:

* ``Trial`` - You must explicitly opt-in to use this feature.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add a column here for a link to context. PR/Feature request thread or something like that. Especially useful for tracking when/why something was rejected/accepted.

Copy link
Member

Choose a reason for hiding this comment

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

+1

* ``Accepted`` - This feature has graduated from an experimental
feature to a fully supported, backwards compatible feature in Chalice.
Accepted features still appear in the table for auditing purposes.
* ``Rejected`` - This feature has been removed.


.. list-table:: Experimental APIs
:header-rows: 1

* - Feature
- Feature Flag Name
- Version Added
- Status
- GitHub Issue(s)
* - :doc:`blueprints`
- ``BLUEPRINTS``
- 1.7.0
- Trial
- `#1023 <https://github.com/aws/chalice/pull/1023>`__,
`#651 <https://github.com/aws/chalice/pull/651>`__


See the `original discussion <https://github.com/aws/chalice/issues/1019>`__
for more background information and alternative proposals.
22 changes: 22 additions & 0 deletions tests/functional/cli/test_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
from chalice.invoke import LambdaInvokeHandler
from chalice.invoke import UnhandledLambdaError
from chalice.awsclient import ReadTimeout
from chalice.deploy.validate import ExperimentalFeatureError


class FakeConfig(object):
Expand Down Expand Up @@ -460,3 +461,24 @@ def test_invoke_does_raise_if_no_function_found(runner, mock_cli_factory):
cli_factory=mock_cli_factory)
assert result.exit_code == 2
assert 'foo' in result.output


def test_error_message_displayed_when_missing_feature_opt_in(runner):
with runner.isolated_filesystem():
cli.create_new_project_skeleton('testproject')
sys.modules.pop('app', None)
with open(os.path.join('testproject', 'app.py'), 'w') as f:
# Rather than pick an existing experimental feature, we're
# manually injecting a feature flag into our app. This ensures
# we don't have to update this test if a feature graduates
# from trial to accepted. The '_features_used' is a "package
# private" var for chalice code.
f.write(
'from chalice import Chalice\n'
'app = Chalice("myapp")\n'
'app._features_used.add("MYTESTFEATURE")\n'
)
os.chdir('testproject')
result = _run_cli_command(runner, cli.package, ['out'])
assert isinstance(result.exception, ExperimentalFeatureError)
assert 'MYTESTFEATURE' in str(result.exception)
19 changes: 18 additions & 1 deletion tests/unit/deploy/test_validate.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@
from chalice import CORSConfig
from chalice.deploy.validate import validate_configuration, validate_routes, \
validate_python_version, validate_route_content_types, \
validate_unique_function_names
validate_unique_function_names, validate_feature_flags, \
ExperimentalFeatureError


def test_trailing_slash_routes_result_in_error():
Expand Down Expand Up @@ -225,3 +226,19 @@ def index():

assert validate_route_content_types(sample_app.routes,
sample_app.api.binary_types) is None


def test_can_validate_feature_flags(sample_app):
# The _features_used is marked internal because we don't want
# chalice users to access it, but this attribute is intended to be
# accessed by anything within the chalice codebase.
sample_app._features_used.add('SOME_NEW_FEATURE')
with pytest.raises(ExperimentalFeatureError):
validate_feature_flags(sample_app)
# Now if we opt in, validation is fine.
sample_app.experimental_feature_flags.add('SOME_NEW_FEATURE')
try:
validate_feature_flags(sample_app)
except ExperimentalFeatureError:
raise AssertionError("App was not suppose to raise an error when "
"opting in to features via a feature flag.")
Loading