[trebuchet] force deployment of trebuchets via admin console#45
[trebuchet] force deployment of trebuchets via admin console#45sarahwada wants to merge 5 commits intoairbnb:masterfrom
Conversation
sarahwada
changed the title
sarahwada
changed the title
CHANGELOG.md
Outdated
| @@ -1,3 +1,6 @@ | |||
| ## 0.10.0 (May 11, 2018) | |||
lib/trebuchet/feature.rb
Outdated
| end | ||
|
|
||
| def aim(strategy_name, options = nil) | ||
| def aim(strategy_name, options = nil, force = false) |
There was a problem hiding this comment.
This PR seems to suggest the force boolean flag is associated with each individual strategy inside a feature.
However, shouldn't such flag be associated with a feature rather than strategies in a feature?
I was under the impression this boolean flag would be implemented in similar ways as comment and expiration_date.
See https://github.com/wang103/trebuchet/blob/f92d20d7a653ae86a4dd944e8a34db9119d82d6f/lib/trebuchet/feature.rb#L117-L121
https://github.com/wang103/trebuchet/blob/f92d20d7a653ae86a4dd944e8a34db9119d82d6f/lib/trebuchet/feature.rb#L117-L121
In other words, just add a setter method named something like def set_force_deploy(force_deploy) and a getter method named something like def force_deploy() in feature.rb?
There was a problem hiding this comment.
ugh, default positional arguments after an options hash is a pretty ugly pattern and we should avoid it.
There was a problem hiding this comment.
Yeah, ideally I would like this force boolean flag to be associated with a specific deployment (a deployment containing multiple strategies or an expiration date).
The problem I was running into is that for each individual component of a deployment is saved as a separate sitar configuration in the backend--one new deployment per strategy changed and expiration date. A force deployment should be associated with a specific configuration, but that information is lost once implementing a force deployment becomes its a separate configuration.
There was a problem hiding this comment.
Given that this entire change is just the added positional argument (which is not needed as Allen mentioned), I can abandon this change. @wang103 let me know if you want to chat more about the implementation of force.
There was a problem hiding this comment.
Ok--looking once again, from my understanding it seems that the options hash isn't actually treated as a free-form options hash, and instead is expected to be a single value containing the strategy (e.g.: https://github.com/airbnb/trebuchet/blob/master/lib/trebuchet/backend/redis.rb#L56). Given this, I'm not sure what the ruby-esque way of implementing the api change. What about changing the optionsparam name to strategy_content for clarity, and then adding a true options hash as the last parameter?
lib/trebuchet/backend/memcached.rb
Outdated
| end | ||
|
|
||
| def set_strategy(feature, strategy, options = nil) | ||
| def set_strategy(feature, strategy, options = nil, force = false) |
There was a problem hiding this comment.
❌ don't use default positional arguments for this. https://github.com/airbnb/ruby#no-default-args
There was a problem hiding this comment.
Ah didn't realize that's what options = nil was. Will remove the force parameter and look for the value in the options hash instead.
rather than embedding in the methods itself. this is due to the fact that the options hash actually just contains the strategy, and as such an optional force parameter cannot be easily added to the hash.
sarahwada
force-pushed
the
swada--trebuchet_force_deployment
branch
from
4b3da38 to
a81497f
Compare
lib/trebuchet/feature.rb
Outdated
| end | ||
|
|
||
| # Set the feature to be force depoyed. | ||
| def set_force() |
There was a problem hiding this comment.
omit parens https://github.com/airbnb/ruby#method-def-parens
lib/trebuchet/feature.rb
Outdated
| end | ||
|
|
||
| def aim(strategy_name, options = nil) | ||
|
|
| Trebuchet.backend.set_expiration_date(self.name, expiration_date) | ||
| end | ||
|
|
||
| # Set the feature to be force depoyed. |
There was a problem hiding this comment.
i don't think this comment adds value (this is what i'd expect from a function called "set force") as is ... but what does it actually mean to be force deployed? would you be able to capture that in a 1-2 line comment?
4 participants
This change adds a method to apply a force field to a trebuchet deployment. This can be handled differently depending on the backend. For the existing backend implementations included in this library, there is no handling of a force deployment.