search

nchammas / flintrock

A command-line tool for launching Apache Spark clusters.
Apache License 2.0
637 stars 116 forks source link

add option to tag ec2 instances #186

Closed lwoloszy closed 7 years ago

lwoloszy commented 7 years ago

This PR allows users to label their EC2 instances with custom tags. An EC2 tag is added with the --ec2-tag option, where each tag is a Key,Value pair separated by a comma. The ec2-tag can be specified multiple times.

I tested this both for launching new clusters and appending slaves to existing clusters. As far as I can tell, it works as expected.

Speaking of the code itself, there's a common formatting step that takes the tags as supplied on the command line and transforms them into a dictionary that EC2 expects. This piece of code is replicated for launching and adding slaves as I didn't know the proper place to put it if we wanted to wrap it as a function (the ec2.py file?). This would be trivial to do if desired.

Cheers, Luke

Fixes #182

lwoloszy commented 7 years ago

Hi @nchammas ,

Glad to contribute! And thanks for the feedback. I've added the changes as requested (which have def streamlined the code). One thing I'm still unsure of is how to allow for tags that have only a Key, but no Value. As it's implemented right now, if a user wants a tag with only a Key, the comma separating the Key from the (implicitly) empty Value is still required. We may want to allow the option to specify a Key without the comma. Once we've made that decision, I'm happy to add a couple of tests.

Luke

lwoloszy commented 7 years ago

Cool, I think we're almost there. Just to make sure, I don't think we want to allow keys that are all whitespace. I've made that explicit in the error message that's thrown.

Whoops, forgot a test case before pushing. if we try to split on a comma before checking that a comma exists, bad things ensue. Perhaps that's why I had decided to do some error checking before parsing the arguments. Let me think about it.

***I think the simplest solution may be to check for a comma before parsing. But I'm open to suggestions.

lwoloszy commented 7 years ago

Great, logic simplified.

As far as the tests go, should I put them in a separate test_ec2.py file?

nchammas commented 7 years ago

As far as the tests go, should I put them in a separate test_ec2.py file?

Yup.

lwoloszy commented 7 years ago

Hi @nchammas,

Here are a couple versions of tests for the ec2 validate_tags function. Not sure which kind you prefer, so I've included both. Not an expert on writing tests, so let me know where things can be improved.

Cheers, Luke

lwoloszy commented 7 years ago

Happy to help out!