Make the Application tag filter optional

[kernow]

Hi,

I'm currently switching from the outdated cap-ec2 to this gem, thanks for creating it.

In our setup we don't need or use Application tags as we only deploy a single application. Our IP addresses change regularly due to server rebuilds which is the reason for using this gem. Would you be open to a PR that makes setting (and filtering by) aws_ec2_application optional?

https://github.com/fernandocarletti/capistrano-aws/blob/master/lib/capistrano/aws/ec2/ec2.rb#L25

Without making this optional we will need to add the Application tag to every one of our ec2 instances which I'd rather avoid having to do.

If you're open to this I'll submit a PR for it.

Thanks

[fernandocarletti]

Hello! I think we could do it, but not changing the current behavior, so it does not break BC.

I suggest a new configuration (filters_override, maybe?) that, if set, replaces the filters as a whole. WDYT?

[kernow]

I was thinking if you don't define aws_ec2_application it wouldn't include it in the filters and if you do it does.

It would maintain BC with one difference in that it would no longer raise an exception when aws_ec2_application is not defined.

If an filters_override option is added I wonder how would that work if aws_ec2_extra_filters is also defined, might get confusing having the two options available.

[fernandocarletti]

That's true. But having an empty application tag seems shady as well. Specially because you can have empty tags in AWS.

Maybe creating the filters_override and deprecate the aws_ec2_extra_filters would be a good option, then we can release a minor version.

[kernow]

I've created a PR for what I was originally thinking as an example as it's easier to show code rather than talk about it 🙂https://github.com/fernandocarletti/capistrano-aws/pull/12

Another option could be to have something like aws_ec2_override_default_filters which would replace the hard coded filters with the ones specified. That would mean we could keep aws_ec2_extra_filters as it is rather than deprecating it.

instance-state-name being set to running looks like something people would always want as you can't deploy to an instance that isn't running. The other 2 hard coded filters aws_ec2_application & aws_ec2_stage I would think are ones that some people will want to override/turn off in favour of their own filters.

Having to override all of the default filters in order to change one of them feels overly complicated as it would require specifying

  {
    name: 'instance-state-name',
    values: ['running']
  }

in the cap file as well.

I wonder if aws_ec2_stage could follow the same pattern as the PR https://github.com/fernandocarletti/capistrano-aws/pull/12. It currently behaves differently than aws_ec2_application as there is no check to see if aws_ec2_stage has been set so this could currently be sent as undefined if not specified in the deploy.rb.

Let me know your thoughts...

[kernow]

Argh, I just noticed set :aws_ec2_application, (proc { fetch(:application) }) which sets the default value so my PR wouldn't work as it is...

[fernandocarletti]

IMHO The idea of the defaults is to have something ready to work without thinking about anyones specific needs, for that we should allow anything to be overridden.

What may make sense here is to move the default filters to a parameter (aws_ec2_default_filters, maybe?), which allow you to override or extend it as you need. This also could get rid of the extra filters and would not require you to mess with any other variable. Does it makes sense to you?

[kernow]

Sound like a good solution, so the defaults file would have something along these lines?

set :aws_ec2_default_filters, (proc {
  [
    {
      name: "tag:#{fetch(:aws_ec2_application_tag)}",
      values: [fetch(:aws_ec2_application)]
    },
    {
      name: "tag:#{fetch(:aws_ec2_stage_tag)}",
      values: [fetch(:aws_ec2_stage)]
    },
    {
      name: 'instance-state-name',
      values: ['running']
    }
  ]
})

[fernandocarletti]

Yes, that's what I was picturing. Wanna give it a shot?

[kernow]

I'll work on it today

[fernandocarletti]

This was fixed by #12 and published at version 1.2.1 (https://rubygems.org/gems/capistrano-aws/versions/1.2.1)

Thank you for contributing!