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

Fix webhook management issue #1

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

gabrieldagama
Copy link

@gabrieldagama gabrieldagama commented Feb 11, 2022

There were two issue with initial implementation for this provider:

1 - On the resource level, client_id, store_hash, created_at and updated_at fields were not persisting on the TF state, meaning that just after creating the resource if you run a terraform plan terraform would return saying that those fields were changed outside terraform management, which wasn't the case. To fix this we basically removed those fields from state as there was no need for them, we just left client_id but that is being managed differently now to fix the second issue.

2 - The second issue was related to updating BigCommerce credentials on the provider, if the project is already live and this change is done, terraform will create new webhooks for the new credentials and won't delete the old ones, therefore leading into our application receiving double webhooks requests. And if the credentials are deleted from BigCommerce we will need to open a BigCommerce suppport request for deleting those webhooks.

To fix this problem we had to move the bigcommerce access_token and client_id properties from the provider to the resource level, meaning that each webhook will declare these properties. I'm aware this change is not ideal as this is suppose to be a general provider for BigCommerce however I could not find another approach for dealing with this problem because terraform does not track state change from the provider perspective, so whenever changes are done on the provider we don't have access to previous data therefore not able to delete old webhooks.

A possible approach was to add properties prev_access_token and prev_client_id to the provider and track this state however this didn't fix the problem because terraform would still create the new ones and we would need to hack terraform resource structure in order for this approach to work, I believe it will increase complexity even further.

The approach on this pull requests would mean that the declaration of a provider is changed to:

provider "bigcommerce" {
  store_hash   = "your-hash"
}

And the declaration of a webhook resource is now:

resource "bigcommerce_webhook" "example" {
  scope       = "store/customer/*"
  client_id    = "your-client-id"
  access_token = "your-access-token"
  destination = "https://foo.bar/webhook"
  is_active   = true

  header {
    key   = "X-My-Header"
    value = "myheadervalue"
  }

It may make sense to rename this provider to something more webhook specifically. This pull request also adds acceptance tests to the repository.

Copy link

@punkstar punkstar left a comment

Choose a reason for hiding this comment

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

Nice!

hookID := d.Get("id").(string)

fmt.Println(c)
Copy link

Choose a reason for hiding this comment

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

@gabrieldagama any idea why this print statement was here in the upstream branch?

Copy link
Author

Choose a reason for hiding this comment

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

No idea... maybe some debug? not sure!

@S48-Mo
Copy link

S48-Mo commented Feb 11, 2022

Not a review but just an observation ... I hate single character variable names 😅 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants