Skip to content
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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

derrike
Copy link
Contributor

@derrike derrike commented Feb 20, 2025

Adds more information to EC2 Instances and Sagemaker notebooks types.

Copy link
Contributor

@LucianoTaranto LucianoTaranto left a 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 {
Copy link
Contributor

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?

Copy link
Contributor Author

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' });
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@derrike derrike force-pushed the feat/improved-instance-docs branch from 549195b to 3ce5402 Compare February 25, 2025 17:03
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.

2 participants