Instance sorting #138
Instance sorting #138
Conversation
| sortMemory = "memory" | ||
| sortName = "instance-type-name" | ||
|
|
||
| sortAscending = "ascending" |
There was a problem hiding this comment.
can you make shorthands for ascending and descending (i.e.asc and desc) in addition to the fully spelled out.
| --profile string AWS CLI profile to use for credentials and config | ||
| -r, --region string AWS Region to use for API requests (NOTE: if not passed in, uses AWS SDK default precedence) | ||
| --sort-direction string Specify the direction to sort in (ascending, descending) (default "ascending") | ||
| --sort-filter string Specify the field to sort by (on-demand-price, spot-price, vcpu, memory, instance-type-name) (default "instance-type-name") |
There was a problem hiding this comment.
nit: what do you think about changing this to sort-by ?
There was a problem hiding this comment.
Ah I agree with this change. sort-by makes more intuitive sense for what the purpose of the flag is.
|
|
||
| // Sorting constants | ||
|
|
||
| sortODPrice = "on-demand-price" |
There was a problem hiding this comment.
I think these should be more generic where you could select an arbitrary jsonpath of the struct def. For example, if I wanted to sort on memory I could do:
ec2-instance-selector --vcpus-min=4 --sort-by='.MemoryInfo.SizeInMiB'maybe using this lib: https://github.com/oliveagle/jsonpath
I think some flag shorthands could be modeled as well, but it needs to be more generic than hardcoded options. Maybe you could just make all quantity fields sortable by inspecting the type and then using the base of the option to determine the sort key.
For example,
ec2-instance-selector --vcpu=4 --sort-by='vcpus'
ec2-instance-selector --vcpu=4 --sort-by='ebs-optimized-basedline-bandwidth'Here's an example of this in the kubectl cli.
https://kubernetes.io/docs/reference/kubectl/cheatsheet/#:~:text=kubectl%20get%20services%20%2D%2Dsort%2Dby%3D.metadata.name
There was a problem hiding this comment.
Woah yea this is a great idea! I'll look into changing sorting to be more generic.
There was a problem hiding this comment.
stretch goal is to allow multiple sort dimensions. For example, if I wanted to sort by memory and then by vcpus when memory is equal, I could do this:
ec2-instance-selector --vcpus-min=4 --sort-by='.MemoryInfo.SizeInMiB, vcpus'
:)
cmd/examples/example1.go
Outdated
| // if you wish to sort the instances based on set filters. | ||
| sortFilter := "instance-type-name" | ||
| sortDirection := "ascending" | ||
| instanceTypesSlice, err = instanceSelector.SortInstanceTypes(instanceTypesSlice, &sortFilter, &sortDirection) |
There was a problem hiding this comment.
thinking on this more, I think that we should make this compatible w/ v2 of instance-selector. The original Filter functions could maintain functionality. In fact, I think it makes sense to decouple sorting from the selector pkg all together. It's trivial to sort the instanceTypesSlice when using the SDK and is probably a better experience since you know exactly how it's operating. So sorting should really be part of the CLI which just be a incremental feature that doesn't break backwards compatibility.
There was a problem hiding this comment.
This does make sense to me how a user of the instance-selector as a library could just manually sort the instance types themselves, so I do agree that sorting could just be part of the CLI. The idea of adding sorting to v2 is also one that I definitely agree with. There is one issue: in order to reduce the number of API calls relating to fetching spot and OD pricing, the pricing caches are only hydrated if the user sets the PricePerHour filter. In the current implementation of sorting, if sorting by pricing is desired, pricing caches will be refreshed if needed, but if we were to leave this to the users, then they will not be able to sort by price in call cases. I feel as though this might go against the user expectation of what information is included in the returned instance types. However, this can be solved by mentioning this characteristic of the PricePerHour filter in the documentation (comments in filtering method, in Filters struct, and possibly the readme). Would just documentation be enough for this or would a different implementation of filtering instance types be necessary?
There was a problem hiding this comment.
I think docs would be fine for now. If there's a strong signal from users that the limitation is hindering the use of the lib, then we can reevaluate at that time.
| // https://github.com/aws/amazon-ec2-instance-selector/blob/main/pkg/selector/outputs/outputs.go | ||
| // Examples of formatted outputs can be found here: | ||
| // https://github.com/aws/amazon-ec2-instance-selector#examples | ||
| maxResults := 10 | ||
| instanceTypesSlice, _, err = outputs.TruncateResults(&maxResults, instanceTypesSlice) |
There was a problem hiding this comment.
I don't think we really need this for outputs. It's trivial to truncate a slice. It's not clear why this returns 3 values too. What error could be returned from a truncation?
There was a problem hiding this comment.
I don't think we really need this for outputs. It's trivial to truncate a slice.
That is a good point. I am going to move TruncateResults to being a private function for the CLI tool to use.
It's not clear why this returns 3 values too. What error could be returned from a truncation?
The 3 values are the truncated list, the number of truncated items, and an error. The error exists to catch invalid maxResults such as negative numbers.
There was a problem hiding this comment.
For this type of func, I'd drop the error and do something reasonable.
For example,
TruncateResults(-1, 5results) => [], 5
TruncateResults(0, 5results) => [], 5
TruncateResults(10, 5results) => [1,2,3,4,5], 0
There was a problem hiding this comment.
Oh that is a very good idea. I'll change the output of TruncateResults to only output the truncated list and the number of truncated items while ensuring that a reasonable output occurs with previously "invalid" maxResults values.
|
Sorting was implemented in the following PR: #146 |
Issue #, if available: #21, #52, #122
Description of changes:
Implemented instance type sorting both in the CLI and as a library function. Done through the function
SortInstanceTypesinselector.go. The CLI tool now has two additional flags:sort-filterandsort-direction.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.