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

Improve Compute Manager data source #976

Merged
merged 2 commits into from
Sep 26, 2023
Merged

Improve Compute Manager data source #976

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d7733b9
Improve Compute Manager data source
annakhm Sep 19, 2023
33fae3e
Merge branch 'master' into compute-manager-ds
annakhm Sep 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions nsxt/data_source_nsxt_compute_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ func dataSourceNsxtComputeManager() *schema.Resource {

Schema: map[string]*schema.Schema{
"id": getDataSourceIDSchema(),
"display_name": getDisplayNameSchema(),
"display_name": getDataSourceExtendedDisplayNameSchema(),
"description": getDescriptionSchema(),
"server": {
Type: schema.TypeString,
Expand All @@ -45,8 +45,6 @@ func dataSourceNsxtComputeManagerRead(d *schema.ResourceData, m interface{}) err
return fmt.Errorf("failed to read ComputeManager %s: %v", objID, err)
}
obj = objGet
} else if objName == "" {
return fmt.Errorf("error obtaining Compute Manager ID or name during read")
} else {
// Get by full name/prefix
objList, err := client.List(nil, nil, nil, nil, nil, nil, nil)
Expand All @@ -57,6 +55,11 @@ func dataSourceNsxtComputeManagerRead(d *schema.ResourceData, m interface{}) err
var perfectMatch []model.ComputeManager
var prefixMatch []model.ComputeManager
for _, objInList := range objList.Results {
if len(objName) == 0 {
// We want to grab single compute manager
perfectMatch = append(perfectMatch, objInList)
Copy link
Collaborator

@GraysonWu GraysonWu Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

So if there are multiple compute managers, an empty display_name will cause an error. I'm wondering if we should add a more informative err msg indicating empty display_name can only be used with single compute manager to reduce the confusion or change the behavior here to just grab the first compute manager or just treat it as a match all for prefixMatch. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think we should decide which manager to grab without further input from the user.
Failing the data source because multiple objects are matching is a behavior present in most data sources in the provider. Regarding error messages - perhaps we should unify these messages across all data sources for consistency?
I can add more info in the doc for the case of compute manager.

continue
}
if strings.HasPrefix(*objInList.DisplayName, objName) {
prefixMatch = append(prefixMatch, objInList)
}
Expand All @@ -66,16 +69,16 @@ func dataSourceNsxtComputeManagerRead(d *schema.ResourceData, m interface{}) err
}
if len(perfectMatch) > 0 {
if len(perfectMatch) > 1 {
return fmt.Errorf("found multiple Compute Managers with name '%s'", objName)
return fmt.Errorf("found multiple Compute Managers matching the criteria")
}
obj = perfectMatch[0]
} else if len(prefixMatch) > 0 {
if len(prefixMatch) > 1 {
return fmt.Errorf("found multiple Compute Managers with name starting with '%s'", objName)
return fmt.Errorf("found multiple Compute Managers matching the criteria")
}
obj = prefixMatch[0]
} else {
return fmt.Errorf("Compute Manager with name '%s' was not found", objName)
return fmt.Errorf("No Compute Manager matches the criteria")
}
}

Expand Down
37 changes: 32 additions & 5 deletions nsxt/data_source_nsxt_compute_manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ import (
)

func TestAccDataSourceNsxtComputeManager_basic(t *testing.T) {
ComputeManagerName := getComputeManagerName()
testResourceName := "data.nsxt_edge_cluster.test"
computeManagerName := getComputeManagerName()
testResourceName := "data.nsxt_compute_manager.test"

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() {
Expand All @@ -23,11 +23,32 @@ func TestAccDataSourceNsxtComputeManager_basic(t *testing.T) {
Providers: testAccProviders,
Steps: []resource.TestStep{
{
Config: testAccNSXComputeManagerReadTemplate(ComputeManagerName),
Config: testAccNSXComputeManagerReadTemplate(computeManagerName),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr(testResourceName, "display_name", ComputeManagerName),
resource.TestCheckResourceAttr(testResourceName, "display_name", computeManagerName),
resource.TestCheckResourceAttrSet(testResourceName, "id"),
resource.TestCheckResourceAttrSet(testResourceName, "server"),
),
},
},
})
}

func TestAccDataSourceNsxtComputeManager_single(t *testing.T) {
testResourceName := "data.nsxt_compute_manager.test"

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() {
testAccOnlyLocalManager(t)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure whether we need testAccTestMP() for these resources or not. Other than this LGTM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we should rename testAccTestMP to testAccTestDeprecated. I think we should include the new fabric resources in the main test suite

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense. I think that I added testAccTestMP to a some fabric resource(s) which I coded. I'll remove these and rename testAccTestMP

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks!

testAccPreCheck(t)
},
Providers: testAccProviders,
Steps: []resource.TestStep{
{
Config: testAccNSXComputeManagerSingleReadTemplate(),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttrSet(testResourceName, "display_name"),
resource.TestCheckResourceAttrSet(testResourceName, "id"),
resource.TestCheckResourceAttrSet(testResourceName, "description"),
resource.TestCheckResourceAttrSet(testResourceName, "server"),
),
},
Expand All @@ -41,3 +62,9 @@ data "nsxt_compute_manager" "test" {
display_name = "%s"
}`, name)
}

func testAccNSXComputeManagerSingleReadTemplate() string {
return `
data "nsxt_compute_manager" "test" {
}`
}
6 changes: 3 additions & 3 deletions website/docs/d/compute_manager.html.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@ description: A Compute Manager data source.
# nsxt_compute_manager

This data source provides information about a Compute Manager configured on NSX.
When only single Compute Manager is present on NSX, no need to specify any filter in the data source (see example below)

## Example Usage

```hcl
data "nsxt_compute_manager" "test_vcenter" {
display_name = "test-vcenter"
}
```

Expand All @@ -26,5 +26,5 @@ data "nsxt_compute_manager" "test_vcenter" {

In addition to arguments listed above, the following attributes are exported:

* `description` - The description of the Compute Manager.
* `server` - IP address or hostname of compute manager.
* `description` - The description of the resource.
* `server` - IP address or hostname of the resource.