add (back) hasSite check to comp group data validation

w0de commented 4 months ago

Intended to address the issue described here.

This restores a site check to computer groups validation. I've amended the logic so that we shouldn't see any list index exceptions when/if site is entirely empty.

I'm not really sure of several things:

Is this error solely due to these groups + my tf state having been created by a prior version of the provider (0.0.49)?
Why are computers expected when site is not defined, anyways? Can't we just discard them?
Why aren't I seeing a populated computers array in state, if they exist in the schema diff? I'd been assuming that if the error didn't occur, they'd appear after an apply.
Are we using CustomizeDiff correctly?
Why was the 'fix' reverted in the first place (asked humbly)? My assumption is because of the exception I'm now trying to avoid*, but perhaps there's some other cause?

* That exception:

panic: runtime error: index out of range [0] with length 0

goroutine 430 [running]:
github.com/deploymenttheory/terraform-provider-jamfpro/internal/endpoints/computergroups.validateComputersNotAllowedWithSmart({0x0?, 0x0?}, 0x1400074c680, {0x0?, 0x0?})
    /Users/w0de/src/terraform-provider-jamfpro-w0de/internal/endpoints/computergroups/computergroups_data_validation.go:27 +0x1b8
github.com/deploymenttheory/terraform-provider-jamfpro/internal/endpoints/computergroups.customDiffComputeGroups({0x101b86890, 0x14000eb1b90}, 0x1400074c680, {0x101924e60, 0x14000156620})
    /Users/w0de/src/terraform-provider-jamfpro-w0de/internal/endpoints/computergroups/computergroups_data_validation.go:13 +0x30
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.schemaMap.Diff(0x1400043c3c0, {0x101b86890, 0x14000eb1b90}, 0x1400079b6c0, 0x14000549130, 0x101b6d2d0, {0x101924e60, 0x14000156620}, 0x0)
    /Users/w0de/go/pkg/mod/github.com/hashicorp/terraform-plugin-sdk/v2@v2.33.0/helper/schema/schema.go:698 +0x3b8
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*Resource).SimpleDiff(0x101b86af8?, {0x101b86890?, 0x14000eb1b90?}, 0x1400079b6c0, 0x14000eb1bc0?, {0x101924e60?, 0x14000156620?})
    /Users/w0de/go/pkg/mod/github.com/hashicorp/terraform-plugin-sdk/v2@v2.33.0/helper/schema/resource.go:962 +0x9c
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*GRPCProviderServer).PlanResourceChange(0x140003fe720, {0x101b86890?, 0x14000eb1ad0?}, 0x14000548af0)
    /Users/w0de/go/pkg/mod/github.com/hashicorp/terraform-plugin-sdk/v2@v2.33.0/helper/schema/grpc_provider.go:798 +0x824
github.com/hashicorp/terraform-plugin-go/tfprotov5/tf5server.(*server).PlanResourceChange(0x140003fc0a0, {0x101b86890?, 0x14000eb12f0?}, 0x1400064c700)
    /Users/w0de/go/pkg/mod/github.com/hashicorp/terraform-plugin-go@v0.22.0/tfprotov5/tf5server/server.go:811 +0x2b4
github.com/hashicorp/terraform-plugin-go/tfprotov5/internal/tfplugin5._Provider_PlanResourceChange_Handler({0x101b326e0, 0x140003fc0a0}, {0x101b86890, 0x14000eb12f0}, 0x1400089a680, 0x0)
    /Users/w0de/go/pkg/mod/github.com/hashicorp/terraform-plugin-go@v0.22.0/tfprotov5/internal/tfplugin5/tfplugin5_grpc.pb.go:500 +0x1c0
google.golang.org/grpc.(*Server).processUnaryRPC(0x140001dd200, {0x101b86890, 0x14000eb1260}, {0x101b8c940, 0x14000427ba0}, 0x140009777a0, 0x1400043d7a0, 0x102424520, 0x0)
    /Users/w0de/go/pkg/mod/google.golang.org/grpc@v1.61.1/server.go:1385 +0xb40
google.golang.org/grpc.(*Server).handleStream(0x140001dd200, {0x101b8c940, 0x14000427ba0}, 0x140009777a0)
    /Users/w0de/go/pkg/mod/google.golang.org/grpc@v1.61.1/server.go:1796 +0xc00
google.golang.org/grpc.(*Server).serveStreams.func2.1()
    /Users/w0de/go/pkg/mod/google.golang.org/grpc@v1.61.1/server.go:1029 +0x8c
created by google.golang.org/grpc.(*Server).serveStreams.func2 in goroutine 15
    /Users/w0de/go/pkg/mod/google.golang.org/grpc@v1.61.1/server.go:1040 +0x13c

ShocOne commented 4 months ago

@thejoeker12 , your thoughts on this ?

smithjw commented 4 months ago

Just some testing from the Jamf API around how groups will appear when configured differently

Smart Group - No Criteria - No Site

<?xml version="1.0" encoding="UTF-8"?>
  <computer_group>
    <id>1</id>
    <name>Smart Group - No Criteria - No Site</name>
    <is_smart>true</is_smart>
    <site>
      <id>-1</id>
      <name>None</name>
    </site>
    <criteria>
      <size>0</size>
    </criteria>
    <computers>
      <size>2</size>
      <computer>
        <id>2</id>
        <name>Admin’s Virtual Machine</name>
        <mac_address>EA:C2:AE:67:E3:42</mac_address>
        <alt_mac_address>62:D4:E8:1A:B1:18</alt_mac_address>
        <serial_number>ZH24QT6HM9</serial_number>
      </computer>
      <computer>
        <id>3</id>
        <name>admin’s Virtual Machine</name>
        <mac_address>16:35:03:41:39:6B</mac_address>
        <alt_mac_address>DA:93:27:EE:D2:58</alt_mac_address>
        <serial_number>Z46916D5X3</serial_number>
      </computer>
    </computers>
  </computer_group>

Smart Group - With Criteria - No Site

<?xml version="1.0" encoding="UTF-8"?>
  <computer_group>
    <id>2</id>
    <name>Smart Group - With Criteria - No Site</name>
    <is_smart>true</is_smart>
    <site>
      <id>-1</id>
      <name>None</name>
    </site>
    <criteria>
      <size>1</size>
      <criterion>
        <name>Model</name>
        <priority>0</priority>
        <and_or>and</and_or>
        <search_type>like</search_type>
        <value>Mac</value>
        <opening_paren>false</opening_paren>
        <closing_paren>false</closing_paren>
      </criterion>
    </criteria>
    <computers>
      <size>2</size>
      <computer>
        <id>2</id>
        <name>Admin’s Virtual Machine</name>
        <mac_address>EA:C2:AE:67:E3:42</mac_address>
        <alt_mac_address>62:D4:E8:1A:B1:18</alt_mac_address>
        <serial_number>ZH24QT6HM9</serial_number>
      </computer>
      <computer>
        <id>3</id>
        <name>admin’s Virtual Machine</name>
        <mac_address>16:35:03:41:39:6B</mac_address>
        <alt_mac_address>DA:93:27:EE:D2:58</alt_mac_address>
        <serial_number>Z46916D5X3</serial_number>
      </computer>
    </computers>
  </computer_group>

Smart Group - No Criteria - With Site

<?xml version="1.0" encoding="UTF-8"?>
  <computer_group>
    <id>3</id>
    <name>Smart Group - No Criteria - With Site</name>
    <is_smart>true</is_smart>
    <site>
      <id>1</id>
      <name>Test</name>
    </site>
    <criteria>
      <size>0</size>
    </criteria>
    <computers>
      <size>1</size>
      <computer>
        <id>2</id>
        <name>Admin’s Virtual Machine</name>
        <mac_address>EA:C2:AE:67:E3:42</mac_address>
        <alt_mac_address>62:D4:E8:1A:B1:18</alt_mac_address>
        <serial_number>ZH24QT6HM9</serial_number>
      </computer>
    </computers>
  </computer_group>

Smart Group - With Criteria - With Site

<?xml version="1.0" encoding="UTF-8"?>
  <computer_group>
    <id>4</id>
    <name>Smart Group - With Criteria - With Site</name>
    <is_smart>true</is_smart>
    <site>
      <id>1</id>
      <name>Test</name>
    </site>
    <criteria>
      <size>1</size>
      <criterion>
        <name>Model</name>
        <priority>0</priority>
        <and_or>and</and_or>
        <search_type>like</search_type>
        <value>Mac</value>
        <opening_paren>false</opening_paren>
        <closing_paren>false</closing_paren>
      </criterion>
    </criteria>
    <computers>
      <size>1</size>
      <computer>
        <id>2</id>
        <name>Admin’s Virtual Machine</name>
        <mac_address>EA:C2:AE:67:E3:42</mac_address>
        <alt_mac_address>62:D4:E8:1A:B1:18</alt_mac_address>
        <serial_number>ZH24QT6HM9</serial_number>
      </computer>
    </computers>
  </computer_group>

Static Group - With Site

<?xml version="1.0" encoding="UTF-8"?>
  <computer_group>
    <id>5</id>
    <name>Static Group - With Site</name>
    <is_smart>false</is_smart>
    <site>
      <id>1</id>
      <name>Test</name>
    </site>
    <criteria>
      <size>0</size>
    </criteria>
    <computers>
      <size>1</size>
      <computer>
        <id>2</id>
        <name>Admin’s Virtual Machine</name>
        <mac_address>EA:C2:AE:67:E3:42</mac_address>
        <alt_mac_address>62:D4:E8:1A:B1:18</alt_mac_address>
        <serial_number>ZH24QT6HM9</serial_number>
      </computer>
    </computers>
  </computer_group>

Static Group - No Site

<?xml version="1.0" encoding="UTF-8"?>
  <computer_group>
    <id>6</id>
    <name>Static Group - No Site</name>
    <is_smart>false</is_smart>
    <site>
      <id>-1</id>
      <name>None</name>
    </site>
    <criteria>
      <size>0</size>
    </criteria>
    <computers>
      <size>1</size>
      <computer>
        <id>2</id>
        <name>Admin’s Virtual Machine</name>
        <mac_address>EA:C2:AE:67:E3:42</mac_address>
        <alt_mac_address>62:D4:E8:1A:B1:18</alt_mac_address>
        <serial_number>ZH24QT6HM9</serial_number>
      </computer>
    </computers>
  </computer_group>

thejoeker12 commented 4 months ago

Will not merge yet as likely to rewrite all of this logic today to bring up to latest patterns.

ShocOne commented 4 months ago

Howdy, release v0.0.58 has the split now for smart and static computer groups. Which simplifies the stating complexity. The site block is optional in .hcl and only stated for non null values. Aka not -1, etc. I'll reject the PR based on the direction we are now taking with this resource type. Thanks

deploymenttheory / terraform-provider-jamfpro