-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Changes from 2 commits
30062f8
a252d1e
297e222
c66efde
2142c71
96b42a9
ea22664
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,34 @@ | |
from chalice.config import Config # noqa | ||
|
||
|
||
class ExperimentalFeatureError(Exception): | ||
_ERROR_MSG = ( | ||
jamesls marked this conversation as resolved.
Show resolved
Hide resolved
|
||
'You are using experimental features without explicitly opting in.\n' | ||
'Experimental features do not guarantee backwards compatibility ' | ||
'and may be removed in the future.\n' | ||
'If you still like to use these ' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you would |
||
'experimental features, you can opt in by adding this to your ' | ||
'app.py file:\n\n%s\n\n' | ||
'See https://chalice.readthedocs.io/en/latest/topics/experimental.rst' | ||
jamesls marked this conversation as resolved.
Show resolved
Hide resolved
|
||
' for more details.' | ||
) | ||
|
||
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' | ||
jamesls marked this conversation as resolved.
Show resolved
Hide resolved
|
||
'])\n' % ', '.join(["'%s'" % feature | ||
for feature in missing_features])) | ||
return self._ERROR_MSG % opt_in_line | ||
|
||
|
||
def validate_configuration(config): | ||
# type: (Config) -> None | ||
"""Validate app configuration. | ||
|
@@ -23,6 +51,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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,6 +65,7 @@ Topics | |
topics/purelambda | ||
topics/blueprints | ||
topics/cd | ||
topics/experimental | ||
|
||
|
||
API Reference | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
* - :doc:`blueprints` | ||
- ``BLUEPRINTS`` | ||
- 1.7.0 | ||
- Trial | ||
|
||
|
||
See the `original discussion <https://github.com/aws/chalice/issues/1019>`__ | ||
for more background information and alternative proposals. |
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 calledinternal_features_used
.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).