Updates in deployfish.yml not respected for certain properties

[acburdine]

Ran into this case when I was attempting to update the vpc_configuration property of a service in deployfish.yml.

Specifically, I had changed the security_group list inside the service's vpc_configuration property, but upon running the deploy command, I noticed the updated security group list hadn't been applied. Looking through the codebase the reason the updated list wasn't applied seems to be this line: https://github.com/caltechads/deployfish/blob/master/deployfish/aws/ecs.py#L1112-L1113 - where the vpc_configuration block from the deployfish.yml is wiped out in favor of the configuration from the existing service.

The easy fix would be to just remove the two lines mentioned above entirely, but looking through the rest of that file it appears that the pattern of overwriting the yml configuration with the values from the service is repeated for most of the @property declarations in the file. It seems to me that the values from deployfish.yml should always take precedence - even if, say, the yml doesn't have a particular property and the existing service does, because in that case you could infer that the user wished to remove that particular property.

So, I'm opening this issue to ask the question - is there a particular reason to use the existing service's values over the ones defined in deployfish.yml? Before I open a PR I want to make sure that I'm not missing some perfectly valid reason for that override pattern to exist.

[acburdine]

On second thought - I guess the pattern does make sense in some cases, for example:

• an ecs service is created with a base configuration in terraform
• deployfish is only used to update the task definition rather than the entire service, meaning you don't have to put any of the complex configurations of the service into deployfish.yml

So, an updated solution to this issue is maybe to use the configuration already present in the service if it doesn't exist in deployfish.yml? For the above vpc_configuration block that would mean something like:

if self.__aws_service and self.__aws_service['vpc_configuration'] and not self.__vpc_configuration:
    self.__vpc_configuration = self.__aws_service['networkConfiguration']['awsvpcConfiguration']

[cmalek]

We use mostly immutable ECS service parameters because most parameters for a service (service_name, service_role_arn, cluster_name, load_balancer, etc.) can't be changed without deleting and recreating the service. The one we did care about, count, we added the deploy scale command for. And I think I think we correctly handle changes to maximumPercent and minimumHealthyPercent.

But you are right, we need to be able to do that for vpc_configuration.

If you want to try a pull request for this, I'll be happy to review it. Otherwise I can schedule this for sometime in the next few weeks.