feat: add azure devops servicehook webhook resource #461
Conversation
|
|
iswym
force-pushed
the
azuredevops_servicehooks_webhook
branch
3 times, most recently
from
bb46b23 to
6856988
Compare
… for azuredevops_servicehook_webhook resource
iswym
force-pushed
the
azuredevops_servicehooks_webhook
branch
from
6856988 to
ebcc1ce
Compare
There was a problem hiding this comment.
@iswym Thanks for your PR, a few changes requested. I was concerning that this resource support so many event type and filter and all of the have an internal name for them, if user set the wrong name, service will return an error and won't be able to create this resource. It would be great if we can document the event types and filer names in the document.
| "updated_at": { | ||
| Type: schema.TypeString, | ||
| Computed: true, | ||
| ForceNew: true, |
There was a problem hiding this comment.
Considering this property is a Computed, ForceNew should removed.
| "url": { | ||
| Type: schema.TypeString, | ||
| Required: true, | ||
| ValidateFunc: validation.IsURLWithHTTPS, |
There was a problem hiding this comment.
Suggest ValidateFunc: validation.IsURLWithHTTPorHTTPS, , in case of HTTP URL.
| return nil | ||
| } | ||
|
|
||
| func getSubscription(d *schema.ResourceData) servicehooks.Subscription { |
There was a problem hiding this comment.
| func getSubscription(d *schema.ResourceData) servicehooks.Subscription { | |
| func expandSubscription(d *schema.ResourceData) servicehooks.Subscription { |
| }) | ||
|
|
||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Error message should return more context information which will help to locate the issue.
For example: return fmt.Errorf(" Create Web Hook Comsumer failed. EventType: %s, ComsumerID: %s. Error: %+v ", *subscription.EventType, *subscriptionData.ConsumerId, err)
| subscriptionId := d.Id() | ||
|
|
||
| subscription, err := clients.ServiceHooksClient.GetSubscription(clients.Ctx, servicehooks.GetSubscriptionArgs{ | ||
| SubscriptionId: converter.UUID(subscriptionId), |
There was a problem hiding this comment.
subscriptionId can be replace by d.Id()
| }) | ||
|
|
||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Same like above, error message should return more context information which will help to locate the issue.
| basicAuth["username"] = username | ||
| } | ||
|
|
||
| if password, ok := basicAuth["password"]; ok && !afterCreateOrUpdate && oldUpdatedAt != newUpdatedAt { |
There was a problem hiding this comment.
afterCreateOrUpdatecan be removed. The firstd.Get("updated_at"),oldUpdatedAtwill be an empty string, this can be used to determine if thepasswordhas been updated out of terraform- Any configuration changing in this resource will update the
updated_at, not only forpassword. We cannot detect the password changes.
| { | ||
| Config: hclSvcHookWebhookResourceBasic(projectName, eventType, url), | ||
| Check: resource.ComposeTestCheckFunc( | ||
| testutils.CheckServiceHookWebhookExistsWithEventTypeAndUrl(tfSvcHookNode, eventType, url), |
There was a problem hiding this comment.
Duplicate with resource.TestCheckResourceAttr(tfSvcHookNode, "event_type", eventType), and resource.TestCheckResourceAttr(tfSvcHookNode, "url", url),
| - `event_type` - (Required) Event type. | ||
| - `url` - (Required) The url of the hook to invoke. | ||
| - `basic_auth` - (Optional) Basic authentication. | ||
| - `username` |
There was a problem hiding this comment.
The description of username and password are empty.
|
|
||
| ## Relevant Links | ||
|
|
||
| - [Azure DevOps Service REST API 5.1 - Service hooks](https://docs.microsoft.com/en-us/rest/api/azure/devops/hooks/?view=azure-devops-rest-5.1) |
There was a problem hiding this comment.
-
This resource support import. This should documented in this document.
Ref : https://github.com/microsoft/terraform-provider-azuredevops/blob/main/website/docs/r/serviceendpoint_bitbucket.html.markdown -
This doc should be added to https://github.com/microsoft/terraform-provider-azuredevops/blob/main/website/azuredevops.erb
|
Since the provider was upgraded to API v6 a rebase must be done in addition to the requested changes by @xuzhang3 |
|
Any news on this? - We're really missing the service hook functionality :) |
All Submissions:
What about the current behavior has changed?
Added:
azuredevops_servicehook_webhookazuredevops_servicehook_webhookresourceazuredevops_servicehook_webhookresourceDoes this introduce a change to
go.mod,go.sumorvendor/?reason:
azure-devops-go-apidependencyDoes this introduce a breaking change?
Any relevant logs, error output, etc?
Other information
Locally built and tested manually.