-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: improved instance docs #45
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - got a couple comments to get your thoughts on if you don't mind
/** | ||
* The EC2 attributes that come directly from the pricing API. | ||
*/ | ||
interface EC2InstanceAttributes { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anywhere we can get these from instead of maintaining them ourselves?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well... they are available from the Pricing API. This could be another automatically generated Type available in the type lib. But, this would be a case of we'd need that to be available first so we could use it. For now, I think we keep it as-is and expand further in the type lib unless you disagree
let allInstanceTypes: InstanceTypeInfo[] = []; | ||
let nextToken: string | undefined; | ||
// Create Pricing client | ||
const pricingClient = new Pricing({ region: 'us-east-1' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know I ask every time but what do we think about this - do we think instance types and their product attributes might be regional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed they are different per region which warrants a bigger discussion.
549195b
to
3ce5402
Compare
Adds more information to EC2 Instances and Sagemaker notebooks types.