AvailabilityZone is optional for Subnet

[adamfokken]

AWS CF Subnet Doc

[bkrodgers]

Thanks for the submission, Adam! What do you think about adding an apply method with the old signature for backwards compatibility? You could add it as a deprecated method that we'd eventually remove, but that would keep it from being a breaking change.

On a related note, looks like you missed the unit tests. They're not compiling. I'm thinking that even with the backwards compatibility change, it would be good to change those over to use Option. Or you could leave them an add additional tests for the Option method, and we'd then remove the non-Option versions if/when we remove the backwards compatibility apply().

[adamfokken]

Yep! I'll clean this up. I missed the unit tests.

[adamfokken]

From my understanding, Scala only allows one apply method to have defaults arguments. This would require the new signature to explicitly define all the arguments.

http://stackoverflow.com/questions/4652095/why-does-the-scala-compiler-disallow-overloaded-methods-with-default-arguments

Do you think that's the right way forward?

[bkrodgers]

Ahh, you're right...I always forget that rule. I'd rather not lose the ability to have default arguments on the "new" signature. But that makes avoiding a breaking change a problem.

Two approaches we could take:

1. Add a factory style method with a different name for the "new" signature, leaving the old one as the only "apply" method. If you look at what I did on AWS::EC2::EIP, you'll see that kind of approach to avoiding a breaking change. This doesn't perfectly fit this situation though, since the ideal case for those factory style methods is when (as we often do in CF) we have lots of mutually exclusive arguments and are trying to avoid invalid combos. That said, it'd work fine here even though we'd only have one factory style method.
2. Just go with a breaking change. It's not the end of the world.

@tj-corrigan any thoughts?

[adamfokken]

This might not be the right forum for this question but I'm curious why you might be qualifying the AZ for the subnet? Are there other dependancies you have and need it specified?

[tjcorr]

I'm fairly certain we are explicitly setting the AZ in order to maximize our usage of reserved instances which are tied to an AZ not a region.

[tjcorr]

Also in general I would try to avoid breaking changes as much as we can. In this case though I'm guessing that people might normally be using the Builders methods so perhaps we can try and make those backwards compatible?

[adamfokken]

Let me know if this is what you're thinking in the builder section? I haven't added deprecation annotations yet.

[bkrodgers]

I don't think we need to change this part. When we use withAZ, we're definitely referencing an AZ.

[adamfokken]

Doh! I've fixed it.

[bkrodgers]

Looks good by glancing. While it is a breaking change for people using the raw subnet object, it should be backwards compatible for people using withSubnet now. I want to run a build locally and confirm that it drops into my existing template before I merge it. May not get to that until later in the week or early next.

[bkrodgers]

This looks good. I dropped it into one of my main existing templates (where we use the builders) and it was backward compatible.

Got a few more PRs to go through -- will cut a 3.2.0 release soon.