Add support for experimental features #1053

jamesls · 2019-01-28T22:07:25Z

Issue #, if available:

Description of changes:

This adds support for feature flags, as discussed here: #1019 (comment)

This PR branches on top of (and targets) the blueprints branch, so after we merge this PR, we can merge blueprints back to master.

Proposed in aws#1049 aws#1019 (comment)

jamesls · 2019-01-28T22:57:08Z

The doc linkchecker keeps failing on travis, though I've manually verified that the links work. I've added retries to this link checker: #1054

kyleknap

Looks fine. Just had a comment and a question.

chalice/deploy/validate.py

kyleknap · 2019-01-28T23:12:00Z

chalice/app.py

+        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()


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.

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.

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

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).

stealthycoin

Code looks good. Just had some questions about tangential stuff like docs.

chalice/deploy/validate.py

stealthycoin · 2019-01-29T18:25:25Z

chalice/deploy/validate.py

+    validate_feature_flags(config.chalice_app)
+
+
+def validate_feature_flags(chalice_app):


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.

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

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.

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

stealthycoin · 2019-01-29T18:30:38Z

docs/source/topics/experimental.rst

+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.


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.

JordonPhillips · 2019-01-29T22:02:07Z

docs/source/topics/experimental.rst

+    $ 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:


If you would

JordonPhillips · 2019-01-29T22:02:38Z

chalice/deploy/validate.py

+        '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 '


If you would

JordonPhillips · 2019-01-29T22:03:31Z

chalice/deploy/validate.py

+    validate_feature_flags(config.chalice_app)
+
+
+def validate_feature_flags(chalice_app):


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

JordonPhillips · 2019-01-29T22:05:35Z

docs/source/topics/experimental.rst

+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.


JordonPhillips · 2019-01-29T22:07:24Z

Looks great!

jamesls

Feedback incorporated, I'll take a look at what codecov is complaining about.

chalice/deploy/validate.py

jamesls · 2019-01-29T22:31:07Z

chalice/deploy/validate.py

+    validate_feature_flags(config.chalice_app)
+
+
+def validate_feature_flags(chalice_app):


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.

codecov-io · 2019-01-29T22:35:25Z

Codecov Report

Merging #1053 into blueprints will increase coverage by <.01%.
The diff coverage is 86.2%.

@@              Coverage Diff               @@
##           blueprints    #1053      +/-   ##
==============================================
+ Coverage       95.45%   95.45%   +<.01%     
==============================================
  Files              27       27              
  Lines            4528     4556      +28     
  Branches          569      574       +5     
==============================================
+ Hits             4322     4349      +27     
- Misses            134      135       +1     
  Partials           72       72

Impacted Files	Coverage Δ
chalice/app.py	`96.99% <100%> (+0.49%)`	⬆️
chalice/deploy/validate.py	`100% <100%> (ø)`	⬆️
chalice/constants.py	`100% <100%> (ø)`	⬆️
chalice/cli/__init__.py	`86.46% <25%> (-0.94%)`	⬇️
chalice/cli/factory.py	`92.35% <75%> (-0.51%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53cfc47...ea22664. Read the comment docs.

jamesls · 2019-01-29T23:21:37Z

Not sure what codecov/patch is complaining about, otherwise travis is passing now.

cc @kyleknap @stealthycoin

kyleknap

Yeah I'm not sure why Codecov is saying we are missing coverage for that line. We have a test for the CLI exception. Otherwise 🚢

jamesls added 2 commits January 28, 2019 13:49

Add support for experimental features

30062f8

Proposed in aws#1049 aws#1019 (comment)

Add documentation for feature flags

a252d1e

jamesls requested review from dstufft, stealthycoin, joguSD, JordonPhillips and kyleknap January 28, 2019 22:07

kyleknap reviewed Jan 28, 2019

View reviewed changes

Change link from rst to html

297e222

stealthycoin reviewed Jan 29, 2019

View reviewed changes

Move error message to constant

c66efde

JordonPhillips reviewed Jan 29, 2019

View reviewed changes

Update docs with a link to GH issues

2142c71

jamesls commented Jan 29, 2019

View reviewed changes

jamesls added 2 commits January 29, 2019 14:52

Fix linting error for line spacing

96b42a9

Add feature opt in test through CLI layer

ea22664

kyleknap approved these changes Jan 30, 2019

View reviewed changes

JordonPhillips approved these changes Jan 30, 2019

View reviewed changes

stealthycoin approved these changes Jan 30, 2019

View reviewed changes

jamesls merged commit ea22664 into aws:blueprints Jan 30, 2019

This was referenced Jan 30, 2019

Merge blueprints branch back to master #1057

Merged

Support experimental/provisional APIs #1019

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for experimental features #1053

Add support for experimental features #1053

jamesls commented Jan 28, 2019

jamesls commented Jan 28, 2019

kyleknap left a comment

kyleknap Jan 28, 2019

jamesls Jan 29, 2019

stealthycoin Jan 29, 2019

kyleknap Jan 29, 2019

stealthycoin left a comment

stealthycoin Jan 29, 2019

JordonPhillips Jan 29, 2019

jamesls Jan 29, 2019

stealthycoin Jan 30, 2019

stealthycoin Jan 29, 2019

JordonPhillips Jan 29, 2019

JordonPhillips Jan 29, 2019

JordonPhillips Jan 29, 2019

JordonPhillips Jan 29, 2019

JordonPhillips Jan 29, 2019

JordonPhillips commented Jan 29, 2019

jamesls left a comment

jamesls Jan 29, 2019

codecov-io commented Jan 29, 2019 •

edited

Loading

jamesls commented Jan 29, 2019 •

edited

Loading

kyleknap left a comment

		validate_feature_flags(config.chalice_app)


		def validate_feature_flags(chalice_app):

Add support for experimental features #1053

Add support for experimental features #1053

Conversation

jamesls commented Jan 28, 2019

jamesls commented Jan 28, 2019

kyleknap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stealthycoin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JordonPhillips commented Jan 29, 2019

jamesls left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 29, 2019 • edited Loading

Codecov Report

jamesls commented Jan 29, 2019 • edited Loading

kyleknap left a comment

Choose a reason for hiding this comment

codecov-io commented Jan 29, 2019 •

edited

Loading

jamesls commented Jan 29, 2019 •

edited

Loading