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

d/ec2_instance_type #13124

Merged
merged 8 commits into from
Oct 7, 2020
Merged

d/ec2_instance_type #13124

merged 8 commits into from
Oct 7, 2020

Conversation

lde
Copy link
Contributor

@lde lde commented May 1, 2020

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #10989

Release note for CHANGELOG:

New datasource ec2_instance_type to get characteristics about ec2 instance type

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccDataSourceAwsEc2InstanceType_attributes'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccDataSourceAwsEc2InstanceType_attributes -timeout 120m
=== RUN   TestAccDataSourceAwsEc2InstanceType_attributes
=== PAUSE TestAccDataSourceAwsEc2InstanceType_attributes
=== CONT  TestAccDataSourceAwsEc2InstanceType_attributes
--- PASS: TestAccDataSourceAwsEc2InstanceType_attributes (35.07s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	35.098s
...

@lde lde requested a review from a team May 1, 2020 14:59
@ghost ghost added size/XL Managed by automation to categorize the size of a PR. needs-triage Waiting for first response or review from a maintainer. documentation Introduces or discusses updates to documentation. provider Pertains to the provider itself, rather than any interaction with AWS. service/ec2 Issues and PRs that pertain to the ec2 service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels May 1, 2020
@lde lde force-pushed the d-aws-instance-type branch 3 times, most recently from ba8bb11 to 776c16b Compare May 1, 2020 15:57
@ewbankkit
Copy link
Contributor

Closes #10989.

@YakDriver
Copy link
Member

@lde Thank you for your effort on this! At first glance, this looks great and I'd like to help get this moving. Would you be willing to rebase?

@YakDriver YakDriver self-assigned this Jul 29, 2020
@lde lde force-pushed the d-aws-instance-type branch 2 times, most recently from 8fe7d71 to b126f97 Compare July 30, 2020 15:46
@lde
Copy link
Contributor Author

lde commented Jul 30, 2020

@lde Thank you for your effort on this! At first glance, this looks great and I'd like to help get this moving. Would you be willing to rebase?

Hi @YakDriver I've rebased my pr, it should be ready to merge.

@iyuroch
Copy link

iyuroch commented Sep 7, 2020

Hi @YakDriver , we need this feature in one of our terraform modules, when can we expect this to be merged in?

Copy link
Member

@YakDriver YakDriver left a comment

Choose a reason for hiding this comment

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

This is looking great! Thank you for your work. It needs a few fixes. Let me know if you're willing to make the changes.

aws/data_source_aws_ec2_instance_type.go Outdated Show resolved Hide resolved
aws/data_source_aws_ec2_instance_type.go Outdated Show resolved Hide resolved
aws/data_source_aws_ec2_instance_type.go Outdated Show resolved Hide resolved
aws/data_source_aws_ec2_instance_type.go Outdated Show resolved Hide resolved
aws/data_source_aws_ec2_instance_type.go Outdated Show resolved Hide resolved
website/docs/d/ec2_instance_type.html.markdown Outdated Show resolved Hide resolved
website/docs/d/ec2_instance_type.html.markdown Outdated Show resolved Hide resolved
website/docs/d/ec2_instance_type.html.markdown Outdated Show resolved Hide resolved
website/docs/d/ec2_instance_type.html.markdown Outdated Show resolved Hide resolved
@YakDriver YakDriver added waiting-response Maintainers are waiting on response from community or contributor. and removed needs-triage Waiting for first response or review from a maintainer. labels Sep 16, 2020
@YakDriver
Copy link
Member

@lde I just wanted to check in with you and see if you are willing to make the changes. If not, we can take it over or someone from the community can. We want to give you the first chance to finish if you're able. 👍

@lde lde force-pushed the d-aws-instance-type branch 4 times, most recently from 0364090 to d3016aa Compare September 23, 2020 21:20
@lde
Copy link
Contributor Author

lde commented Sep 23, 2020

Hi, @YakDriver
I've update pr integrating your changes.

But i've an issue on terrafmt check.
I'm not able to get error on local for debug

terrafmt diff --check --verbose --fmtcompat aws/data_source_aws_ec2_instance_type_test.go
aws/data_source_aws_ec2_instance_type_test.go: 101 lines & 0/0 blocks need formatting.

@ghost ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 23, 2020
@YakDriver YakDriver added the waiting-response Maintainers are waiting on response from community or contributor. label Sep 30, 2020
@lde
Copy link
Contributor Author

lde commented Oct 3, 2020

Hi @YakDriver, I've splited tests and added somes missing attributes.

@ghost ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Oct 3, 2020
Copy link
Member

@YakDriver YakDriver left a comment

Choose a reason for hiding this comment

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

This is very close. Just a couple of small items and we should be ready to merge!


~> **NOTE:** Not all attributes are set for every instance type.

* `accelerators` Describes the Inference accelerators for the instance type.
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate these being in alphabetical order! It makes the docs more readable!

`

const testAccDataSourceEc2InstanceTypeAccelerator = `

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

The blank line is probably triggering the linter.

aws/data_source_aws_ec2_instance_type_test.go Show resolved Hide resolved
Comment on lines 364 to 458
d.Set("instance_type", v.InstanceType)
d.Set("current_generation", v.CurrentGeneration)
d.Set("free_tier_eligible", v.FreeTierEligible)
d.Set("supported_usages_classes", v.SupportedUsageClasses)
d.Set("supported_root_device_types", v.SupportedRootDeviceTypes)
d.Set("supported_virtualization_types", v.SupportedVirtualizationTypes)
d.Set("bare_metal", v.BareMetal)
d.Set("hypervisor", v.Hypervisor)
d.Set("supported_architectures", v.ProcessorInfo.SupportedArchitectures)
d.Set("sustained_clock_speed", v.ProcessorInfo.SustainedClockSpeedInGhz)
d.Set("default_vcpus", v.VCpuInfo.DefaultVCpus)
d.Set("default_cores", v.VCpuInfo.DefaultCores)
d.Set("default_threads_per_core", v.VCpuInfo.DefaultThreadsPerCore)
d.Set("valid_threads_per_core", v.VCpuInfo.ValidThreadsPerCore)
d.Set("valid_cores", v.VCpuInfo.ValidCores)
d.Set("memory_size", v.MemoryInfo.SizeInMiB)
d.Set("instance_storage_supported", v.InstanceStorageSupported)
if v.InstanceStorageInfo != nil {
d.Set("total_instance_storage", v.InstanceStorageInfo.TotalSizeInGB)
if v.InstanceStorageInfo.Disks != nil {
diskList := make([]interface{}, len(v.InstanceStorageInfo.Disks))
for i, dk := range v.InstanceStorageInfo.Disks {
disk := map[string]interface{}{
"size": aws.Int64Value(dk.SizeInGB),
"count": aws.Int64Value(dk.Count),
"type": aws.StringValue(dk.Type),
}
diskList[i] = disk
}
d.Set("instance_disks", diskList)
}
}
d.Set("ebs_optimized_support", v.EbsInfo.EbsOptimizedSupport)
if v.EbsInfo.EbsOptimizedInfo != nil {
d.Set("ebs_performance_baseline_bandwidth", v.EbsInfo.EbsOptimizedInfo.BaselineBandwidthInMbps)
d.Set("ebs_performance_baseline_throughput", v.EbsInfo.EbsOptimizedInfo.BaselineThroughputInMBps)
d.Set("ebs_performance_baseline_iops", v.EbsInfo.EbsOptimizedInfo.BaselineIops)
d.Set("ebs_performance_maximum_bandwidth", v.EbsInfo.EbsOptimizedInfo.MaximumBandwidthInMbps)
d.Set("ebs_performance_maximum_throughput", v.EbsInfo.EbsOptimizedInfo.MaximumThroughputInMBps)
d.Set("ebs_performance_maximum_iops", v.EbsInfo.EbsOptimizedInfo.MaximumIops)
}
d.Set("ebs_encryption_support", v.EbsInfo.EncryptionSupport)
d.Set("ebs_nvme_support", v.EbsInfo.NvmeSupport)
d.Set("network_performance", v.NetworkInfo.NetworkPerformance)
d.Set("maximum_network_interfaces", v.NetworkInfo.MaximumNetworkInterfaces)
d.Set("maximum_ipv4_addresses_per_interface", v.NetworkInfo.Ipv4AddressesPerInterface)
d.Set("maximum_ipv6_addresses_per_interface", v.NetworkInfo.Ipv6AddressesPerInterface)
d.Set("ipv6_supported", v.NetworkInfo.Ipv6Supported)
d.Set("ena_support", v.NetworkInfo.EnaSupport)
d.Set("efa_supported", v.NetworkInfo.EfaSupported)
if v.GpuInfo != nil {
gpuList := make([]interface{}, len(v.GpuInfo.Gpus))
for i, gp := range v.GpuInfo.Gpus {
gpu := map[string]interface{}{
"manufacturer": aws.StringValue(gp.Manufacturer),
"name": aws.StringValue(gp.Name),
"count": aws.Int64Value(gp.Count),
"memory_size": aws.Int64Value(gp.MemoryInfo.SizeInMiB),
}
gpuList[i] = gpu
}
d.Set("gpus", gpuList)
d.Set("total_gpu_memory", v.GpuInfo.TotalGpuMemoryInMiB)
}
if v.FpgaInfo != nil {
fpgaList := make([]interface{}, len(v.FpgaInfo.Fpgas))
for i, fpg := range v.FpgaInfo.Fpgas {
fpga := map[string]interface{}{
"manufacturer": aws.StringValue(fpg.Manufacturer),
"name": aws.StringValue(fpg.Name),
"count": aws.Int64Value(fpg.Count),
"memory_size": aws.Int64Value(fpg.MemoryInfo.SizeInMiB),
}
fpgaList[i] = fpga
}
d.Set("fpgas", fpgaList)
d.Set("total_fpga_memory", v.FpgaInfo.TotalFpgaMemoryInMiB)
}
d.Set("supported_placement_strategies", v.PlacementGroupInfo.SupportedStrategies)
if v.InferenceAcceleratorInfo != nil {
acceleratorList := make([]interface{}, len(v.InferenceAcceleratorInfo.Accelerators))
for i, accl := range v.InferenceAcceleratorInfo.Accelerators {
accelerator := map[string]interface{}{
"manufacturer": aws.StringValue(accl.Manufacturer),
"name": aws.StringValue(accl.Name),
"count": aws.Int64Value(accl.Count),
}
acceleratorList[i] = accelerator
}
d.Set("accelerators", acceleratorList)
}
d.Set("hibernation_supported", v.HibernationSupported)
d.Set("burstable_performance_supported", v.BurstablePerformanceSupported)
d.Set("dedicated_hosts_supported", v.DedicatedHostsSupported)
d.Set("auto_recovery_supported", v.AutoRecoverySupported)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Please put all the attributes in alphabetical order. It makes maintenance easier later!

@YakDriver YakDriver added the waiting-response Maintainers are waiting on response from community or contributor. label Oct 7, 2020
@lde
Copy link
Contributor Author

lde commented Oct 7, 2020

Hi,
If I didn't make mistake all documentation, structures set and tests are sorted.

@ghost ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Oct 7, 2020
Copy link
Member

@YakDriver YakDriver left a comment

Choose a reason for hiding this comment

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

Once we make the linter happy, we are ready to go.

aws/data_source_aws_ec2_instance_type_test.go Outdated Show resolved Hide resolved
aws/data_source_aws_ec2_instance_type_test.go Outdated Show resolved Hide resolved
aws/data_source_aws_ec2_instance_type_test.go Outdated Show resolved Hide resolved
aws/data_source_aws_ec2_instance_type_test.go Outdated Show resolved Hide resolved
aws/data_source_aws_ec2_instance_type_test.go Outdated Show resolved Hide resolved
@YakDriver YakDriver added the waiting-response Maintainers are waiting on response from community or contributor. label Oct 7, 2020
@YakDriver YakDriver added this to the v3.10.0 milestone Oct 7, 2020
Copy link
Member

@YakDriver YakDriver left a comment

Choose a reason for hiding this comment

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

I apologize. I found some additional problems running the tests across partitions.

aws/data_source_aws_ec2_instance_type_test.go Outdated Show resolved Hide resolved
aws/data_source_aws_ec2_instance_type_test.go Outdated Show resolved Hide resolved
aws/data_source_aws_ec2_instance_type_test.go Outdated Show resolved Hide resolved
aws/data_source_aws_ec2_instance_type_test.go Outdated Show resolved Hide resolved
aws/data_source_aws_ec2_instance_type_test.go Outdated Show resolved Hide resolved
@lde
Copy link
Contributor Author

lde commented Oct 7, 2020

Hooray, linter seems to be happy !

@ghost ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Oct 7, 2020
@YakDriver
Copy link
Member

@lde If you have a chance to fix these last few items in the next hour or so, we can get this in in time for 3.10.0.

@YakDriver YakDriver added the waiting-response Maintainers are waiting on response from community or contributor. label Oct 7, 2020
@lde
Copy link
Contributor Author

lde commented Oct 7, 2020

@YakDriver I've applied your tests updates.

@ghost ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Oct 7, 2020
@YakDriver
Copy link
Member

This looks great! Thank you for all your work on this. I think a lot of people will find this useful.

Tests on GovCloud:

--- PASS: TestAccDataSourceAwsEc2InstanceType_basic (15.90s)
--- PASS: TestAccDataSourceAwsEc2InstanceType_gpu (17.59s)
--- PASS: TestAccDataSourceAwsEc2InstanceType_fpga (17.61s)
--- PASS: TestAccDataSourceAwsEc2InstanceType_metal (17.62s)

Tests on Commercial:

--- PASS: TestAccDataSourceAwsEc2InstanceType_basic (11.95s)
--- PASS: TestAccDataSourceAwsEc2InstanceType_metal (12.34s)
--- PASS: TestAccDataSourceAwsEc2InstanceType_fpga (12.36s)
--- PASS: TestAccDataSourceAwsEc2InstanceType_gpu (12.50s)

@YakDriver YakDriver merged commit b1f3fa4 into hashicorp:master Oct 7, 2020
@YakDriver YakDriver added the partition/aws-us-gov Pertains to the aws-us-gov partition. label Oct 7, 2020
@lde lde deleted the d-aws-instance-type branch October 8, 2020 05:10
@ghost
Copy link

ghost commented Oct 9, 2020

This has been released in version 3.10.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

@ghost
Copy link

ghost commented Nov 7, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Nov 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. partition/aws-us-gov Pertains to the aws-us-gov partition. provider Pertains to the provider itself, rather than any interaction with AWS. service/ec2 Issues and PRs that pertain to the ec2 service. size/XL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EC2 instance type discovery
4 participants