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.

Sign up for GitHub

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

Jump to bottom

New Data Source: aws_workspaces_bundle #3243

Merged
merged 1 commit into from
Oct 9, 2018
Merged

New Data Source: aws_workspaces_bundle #3243

merged 1 commit into from
Oct 9, 2018

Conversation

atsushi-ishibashi
Copy link
Contributor

This may be useful for #1917

TF_ACC=1 go test ./aws -v -run=TestAccDataSourceAwsWorkspaceBundle_basic -timeout 120m
=== RUN   TestAccDataSourceAwsWorkspaceBundle_basic
--- PASS: TestAccDataSourceAwsWorkspaceBundle_basic (82.43s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	82.469s

@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 3, 2018
@radeksimko radeksimko added service/workspaces Issues and PRs that pertain to the workspaces service. new-data-source Introduces a new data source. labels Feb 3, 2018
bflad
bflad reviewed Jun 27, 2018
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Hi @atsushi-ishibashi 👋 Thanks for submitting this. I left you some initial feedback below. Can you please take a look and let us know if you have any questions or do not have time to implement the feedback? Thanks!

aws/data_source_aws_workspaces_bundle.go
"name": *bundle.ComputeType.Name,
}
ct := []map[string]interface{}{r}
d.Set("compute_type", ct)
Copy link
Contributor

Choose a reason for hiding this comment

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

When using d.Set() with aggregate types (TypeList, TypeSet, TypeMap), we should perform error checking to prevent issues where the code is not properly able to set the Terraform state. e.g.

if err := d.Set("compute_type", ct); err != nil {
  return fmt.Errorf("error setting compute_type: %s", err)
}

This also needs to be repeated for root_storage and user_storage below.

aws/data_source_aws_workspaces_bundle.go
d.Set("owner", bundle.Owner)
if bundle.ComputeType != nil {
r := map[string]interface{}{
"name": *bundle.ComputeType.Name,
Copy link
Contributor

Choose a reason for hiding this comment

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

To prevent potential panics, we should prefer to use the SDK provided function for dereferencing values (e.g. aws.StringValue(bundle.ComputeType.Name)) instead of directly dereferencing via *

This needs to be repeated for bundle.RootStorage.Capacity and bundle.UserStorage.Capacity below as well.

website/docs/d/workspaces_bundle.html.markdown
Get information on a Workspaces Bundle.
---

# aws_workspaces_bundle
Copy link
Contributor

Choose a reason for hiding this comment

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

Most likely a change that occurred after the creation of this pull request, but can you please prepend the title with Data Source: , e.g. Data Source: aws_workspaces_bundle

aws/data_source_aws_workspaces_bundle.go
"bundle_id": {
Type: schema.TypeString,
Required: true,
ForceNew: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

ForceNew is not required for data sources and should be removed from this attribute and the documentation.

bflad
bflad reviewed Jun 27, 2018
View reviewed changes
aws/config.go
@@ -206,6 +207,7 @@ type AWSClient struct {
athenaconn *athena.Athena
dxconn *directconnect.DirectConnect
mediastoreconn *mediastore.MediaStore
wsconn *workspaces.WorkSpaces
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nitpick: to prevent any confusion with other services, we should probably name this workspacesconn

@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Jun 27, 2018
@bflad bflad mentioned this pull request Oct 9, 2018
Closed
bflad
bflad approved these changes Oct 9, 2018
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

This PR is being merged in with feedback addressed in f370c28 🚀

--- PASS: TestAccDataSourceAwsWorkspaceBundle_basic (8.84s)

@bflad bflad added this to the v1.40.0 milestone Oct 9, 2018
@bflad bflad merged commit 2c8c247 into hashicorp:master Oct 9, 2018
bflad added a commit that referenced this pull request Oct 9, 2018
@bflad
data-source/aws_workspaces_bundle: Address #3243 PR feedback
f370c28 
```
--- PASS: TestAccDataSourceAwsWorkspaceBundle_basic (8.84s)
```
bflad added a commit that referenced this pull request Oct 9, 2018
@bflad
Update CHANGELOG for #3243
2a0076d
@bflad
Copy link
Contributor

bflad commented Oct 10, 2018

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

@ghost
Copy link

ghost commented Apr 2, 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 and limited conversation to collaborators Apr 2, 2020
@breathingdust breathingdust removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bflad bflad bflad approved these changes

Assignees
No one assigned
Labels
new-data-source Introduces a new data source. service/workspaces Issues and PRs that pertain to the workspaces service. size/L Managed by automation to categorize the size of a PR.
Projects
None yet
Milestone
v1.40.0
Development

Successfully merging this pull request may close these issues.
4 participants
@atsushi-ishibashi @bflad @breathingdust @radeksimko