Question about TODOs

[tomahawk360]

Hi! First of all, thanks for developing this tool. Having the experience of building Hadoop clusters "by hand", flintrock has been extremely useful.
I decided to see how flintrock works, and found these TODOs, more specifically the ones about "Centralizing logic" to get ec2 clusters characteristics:

https://github.com/nchammas/flintrock/blob/c3dee6da06af4e6ca094ac6f0b4365d091c2541b/flintrock/ec2.py#L179C1-L206C31

What does "Centralize logic" stands for? What's left to be done in those sections?

[nchammas]

I wrote those comments almost 10 years ago, so I will have to take a guess at what exactly I meant. :)

flintrock/flintrock/ec2.py

Lines 180 to 186 in c3dee6d

	# TODO: Centralize logic to get Flintrock base security group. (?)
	flintrock_base_group = list(
	ec2.security_groups.filter(
	Filters=[
	{'Name': 'group-name', 'Values': ['flintrock']},
	{'Name': 'vpc-id', 'Values': [self.vpc_id]},
	]))[0]

This probably just means I wanted to write a little function that I could reuse across the codebase to get this base security group. I'm not sure how many places would need to be refactored or if such a function is really needed, but I think that's what I meant when I wrote it.

flintrock/flintrock/ec2.py

Lines 199 to 205 in c3dee6d

	# TODO: Centralize logic to get cluster security group name from cluster name.
	cluster_group = list(
	ec2.security_groups.filter(
	Filters=[
	{'Name': 'group-name', 'Values': ['flintrock-' + self.name]},
	{'Name': 'vpc-id', 'Values': [self.vpc_id]},
	]))[0]

This is probably the same idea. There must be several places in the codebase with similar logic, where I'm trying to get the cluster-specific security group based on the cluster name. Instead of repeating that logic, a reusable function would be handy.

[tomahawk360]

Hello, sorry for the late response.

Indeed, the same code bit repeats on three different occasions, each part having the same TODO comment, so it must refer to a replacement of those bits with a reusable function.

However, I found a function that could serve said purpose in the codebase:

flintrock/flintrock/ec2.py

Lines 493 to 516 in c3dee6d

	def get_security_groups(
	*,
	vpc_id,
	region,
	security_group_names) -> "List[boto3.resource('ec2').SecurityGroup]":
	ec2 = boto3.resource(service_name='ec2', region_name=region)
	groups = list(
	ec2.security_groups.filter(
	Filters=[
	{'Name': 'group-name', 'Values': security_group_names},
	{'Name': 'vpc-id', 'Values': [vpc_id]},
	]))
	found_group_names = [group.group_name for group in groups]
	missing_group_names = set(security_group_names) - set(found_group_names)
	if missing_group_names:
	raise Error(
	"Could not find the following security group{s}: {groups}"
	.format(
	s='' if len(missing_group_names) == 1 else 's',
	groups=', '.join(list(missing_group_names))))
	return groups

This function is only used in the launch method, but it could be used for the "Centralize logic" TODOs as well, like for example the first TODO instance could be rewritten as:

flintrock_base_group = get_security_groups(
    vpc_id=self.vpc_id,
    region=self.region,
    security_group_names=['flintrock']
)[0]

Is there a reason why that function is not being used for the "Centralize logic" TODOs?

[tomahawk360]

Hello,
I made the following PR regarding this Issue:
#378

[nchammas]

Thank you. Sorry I have not had a chance to look at this, but it is on my list to do. I just wanted to acknowledge your PR in the meantime.

[tomahawk360]

Hello. No worries, take your time and thanks for having the PR in mind.