WIP update-safe service config for load balancing #83
Conversation
slpsys
changed the title
|
specs are failing |
lib/broadside/target.rb
Outdated
| end | ||
|
|
||
| def update_safe_service_config | ||
| attributes_disallowed_for_updates = %i( |
There was a problem hiding this comment.
constant? SERVICE_ATTRIBUTES_ONLY_SET_AT_INSTANTIATION?
(i don't love that name, feel free to rename, but i do think it should be a constant)
lib/broadside/target.rb
Outdated
| load_balancers | ||
| ) | ||
|
|
||
| service_config.delete_if do |k, _| |
There was a problem hiding this comment.
i know reject and delete_if are identical but we mostly use reject /shrug
There was a problem hiding this comment.
also more importantly, with activesupport i think you can just do service_config.except(attributes_disallowed_for_updates)
lib/broadside/target.rb
Outdated
| } | ||
| end | ||
|
|
||
| def update_safe_service_config |
There was a problem hiding this comment.
maybe this could just be called update_service_config or service_config_for_updates? "safe" doesn't have much explanatory context in this power.
There was a problem hiding this comment.
maybe updateable_service_config, even
There was a problem hiding this comment.
(and add a comment about why these fields are stripped out)
There was a problem hiding this comment.
i mean, this is where cute ruby gets close to english, but isn't english. the intent here is:
service config
vs.
update-safe service config
not "update service config, which is safe"
There was a problem hiding this comment.
going with service_config_for_update, though
|
i'm 👍 with these changes though the other PR i opened #84 addresses them in a slightly less draconian way (you don't need the kind of wonder if GLI has a way of having those two separate commands ( though at the same time, all that feels like a lot of overkill. |
|
in fact it was just 2 commits ago i removed thiswhoops after tim complained it wasn't working. 👻 on me |
apurvis
left a comment
apurvis
left a comment
There was a problem hiding this comment.
this is a good intermediary change no matter what we decide in the end, though i would like those variables/methods renamed slightly
|
Also worth giving a final shoutout to #76 which should eventually solve all ELB issues top to bottom. |
|
i'm on board with the change except for those style nitpicks. cc @matl33t what do you think about this |
| load_balancers | ||
| placement_constraints | ||
| placement_strategy | ||
| role |
There was a problem hiding this comment.
|
Will wait on Matt to merge. |
|
🆒 after some 🕶️ |
apurvis
left a comment
apurvis
left a comment
There was a problem hiding this comment.
lgtm; i guess this is 3.0.5?
|
@apurvis just added more specs and bumped the version |
|
do you want me to merge & release or do you have those privileges? |
|
@apurvis just released |
This is definitely part of #37; create_service needs service config information that tells it about the intended load balancer, and the load balancer info requires an IAM role.
The issue is that while
create-serviceneeds it,update-servicenot only can't change it, but as a result, will error out if it's present. So we need one copy with, one copy without.This doesn't have specs [yet], and also is probably not a complete list [yet].