ryanwholey / terraform-provider-pihole

A Terraform provider for managing Pi-hole resources
https://registry.terraform.io/providers/ryanwholey/pihole/latest/docs
Mozilla Public License 2.0
69 stars 7 forks source link

feat: Add weekly job + fix broken test #70

Closed jlilja closed 3 months ago

jlilja commented 3 months ago

Hello! First off, thank you for creating this Terraform provider. I'm glad that it exists and hope that its features will grow in the future!

I saw that there was an issue regarding adding a weekly job, https://github.com/ryanwholey/terraform-provider-pihole/issues/43. I have added a matrix job that tests for various versions going from the version that is currently being tested up until latest and nightly builds.

When attempting to upgrade from docker tag 2022.05 to 2022.07, the tests started failing. Although I have very little experience doing feature work och troubleshooting terraform providers, I figured I could take a stab at it.

I could narrow down the issue to the TestAccGroups test which, as of 2022.07, started throwing the issue Invalid payload: id is not an array. I narrowed it down even further and found that it fails when attempting to do a delete of the group, so I observed how the request in the ui looks compared to how to go code does it.

POST http://localhost:8080/admin/scripts/pi-hole/php/groups.php Body:

action  "delete_group"
id  "[33]"
token   "REDACTED"

The id is wrapped in square brackets, while in the go code we form the request such as:

req, err := c.RequestWithSession(ctx, "POST", "/admin/scripts/pi-hole/php/groups.php", &url.Values{
    "action": []string{"delete_group"},
    "id":     []string{strconv.FormatInt(toDelete.ID, 10)},
})

That's where it is now incompatible as of pihole 2022.07, when it introduced this piece of validation.

So I rewrote that logic a bit to wrap it into a set of square brackets which made the code happy again.

The test was still a bit grumpy tho, as the post destroy validation function was failing. It was due to this:

if _, ok := err.(*pihole.NotFoundError); !ok {
    return err
}

I believe that the logic should say that if the error is the same as the NotFoundError error, then we can assume that the group has been properly deleted, and we should pass the check. So I flipped the logic to say this instead:

_, err := client.GetGroup(context.Background(), name)
_, sameErr := err.(*pihole.NotFoundError)

if sameErr != true {
    return err
}   

Please let me know if I need to change anything.

jlilja commented 3 months ago

Seems like I can't add reviewers. Ping @ryanwholey :bell:

ryanwholey commented 3 months ago

@jlilja thank you so much for your contribution! Kicked off the tests

ryanwholey commented 3 months ago

Awesome, thanks again!

jlilja commented 3 months ago

Happy to help! :smiley: