Skip to content

Conversation

bwagner5
Copy link
Contributor

Issue #, if available:
N/A

Description of changes:

  • add availability-zones to perform an intersection of availabile instances in multiple zones
  • Deprecate the previous singular --availability-zone

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@bwagner5 bwagner5 requested a review from haugenj June 10, 2020 21:24
@codecov-commenter
Copy link

codecov-commenter commented Jun 10, 2020

Codecov Report

Merging #8 into master will increase coverage by 0.15%.
The diff coverage is 91.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #8      +/-   ##
==========================================
+ Coverage   89.44%   89.60%   +0.15%     
==========================================
  Files           7        7              
  Lines         758      779      +21     
==========================================
+ Hits          678      698      +20     
  Misses         53       53              
- Partials       27       28       +1     
Impacted Files Coverage Δ
pkg/selector/types.go 100.00% <ø> (ø)
pkg/selector/selector.go 85.13% <91.48%> (+1.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93964b8...440299b. Read the comment docs.

Comment on lines 359 to 367
if isZoneID, _ := regexp.MatchString(zoneIDRegex, zone); isZoneID {
instanceTypeOfferingsInput.SetLocationType(zoneIDLocationType)
} else if isZoneName, _ := regexp.MatchString(zoneNameRegex, zone); isZoneName {
instanceTypeOfferingsInput.SetLocationType(zoneNameLocationType)
} else if isRegion, _ := regexp.MatchString(regionNameRegex, zone); isRegion {
instanceTypeOfferingsInput.SetLocationType(regionNameLocationType)
} else {
return nil, fmt.Errorf("The location passed in (%s) is not a valid zone-id, zone-name, or region name", zone)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be nicer to pull this out to a helper func

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call, done!

Comment on lines 370 to 375
if _, ok := availableInstanceTypes[*instanceType.InstanceType]; !ok {
availableInstanceTypes[*instanceType.InstanceType] = 1
} else {
i, _ := availableInstanceTypes[*instanceType.InstanceType]
availableInstanceTypes[*instanceType.InstanceType] = i + 1
}
Copy link
Contributor

@haugenj haugenj Jun 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this isn't the cleanest way to write this since you're splitting i, ok := availableInstanceTypes[*instanceType.InstanceType] into two parts, but my brain isn't working right now to come up with a nicer implementation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ha yeah, you're right! I fixed it since the i is still in scope of the if... :)

h.Ok(t, err)
h.Assert(t, len(results) == 3, "c4.large should return 3 similar instance types")
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens when there's a mix of good + bad AZs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also duplicate AZs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added tests to ensure these cases work 🕺

Copy link
Contributor

@haugenj haugenj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bwagner5 bwagner5 merged commit 8fea12d into aws:master Jun 11, 2020
@bwagner5 bwagner5 deleted the multiple-zones branch July 15, 2020 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants