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

Adding support for servicehooks (azure storage queue consumer) #914

Merged
merged 18 commits into from Dec 13, 2023

Conversation

rdalbuquerque
Copy link
Contributor

@rdalbuquerque rdalbuquerque commented Oct 30, 2023

All Submissions:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • My code follows the code style of this project.
  • I ran lint checks locally prior to submission.
  • Have you checked to ensure there aren't other open PRs for the same update/change?

What about the current behavior has changed?

Adding support for servicehooks with a first resource azuredevops_servicehook_storage_queue

Does this introduce a change to go.mod, go.sum or vendor/?

  • Yes
  • No

servicehooks from azure-devops-go-api wasn't available yet

Does this introduce a breaking change?

  • Yes
  • No

Other information

Servicehooks has consumers and publishers. In the proposed design for Servicehooks, each consumer will have its own dedicated resource. This resource will include a publisher block, allowing users to configure their preferred publisher. The design is intended to be flexible, accommodating new publishers as user needs evolve. Accordingly, new resources will be developed to support these emerging consumers.

I guess there is 4 options:

  1. 1 resource for each consumer with a publisher block
  2. 1 resource for each publisher with a consumer block
  3. 1 resource only with both a consumer block and publisher block
  4. 1 resource for each combination of publisher x consumer

This is currently how the resource looks like:

resource "azuredevops_servicehook_storage_queue" "this" {
  project_id   = data.azuredevops_project.this.id
  account_name = "mystorageacc"
  account_key  = random_string.account_key.result
  queue_name   = "myqueue"
  visi_timeout = 30
  publisher {
    name = "pipelines"
    stage_state_changed {
      state_filter  = "Completed"
      result_filter = "Succeeded"
    }
  }
}

Looking forward to discussing the options and the code 😄

@rdalbuquerque rdalbuquerque changed the title Feat subscription Adding support for servicehooks (azure storage queue consumer) Oct 30, 2023
@xuzhang3
Copy link
Collaborator

@rdalbuquerque can you help add documentation for azuredevops_servicehook_storage_queue

Copy link
Collaborator

@xuzhang3 xuzhang3 left a comment

Choose a reason for hiding this comment

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

@rdalbuquerque Thanks for submit this PR. There is so many consumer and publisher types and they have a relation m <-> m. What I think is add new resource based on the consumer type and publisher type.

@rdalbuquerque
Copy link
Contributor Author

@rdalbuquerque Thanks for submit this PR. There is so many consumer and publisher types and they have a relation m <-> m. What I think is add new resource based on the consumer type and publisher type.

ok so you are proposing we have a resource per publisher type per consumer type.
would this name convention work for you?
azuredevops_servicehook_[publisher_type]_[consumer_type]

if so, this resource would be changed to:

resource "azuredevops_servicehook_pipelines_storage_queue" "this" {
  project_id   = data.azuredevops_project.this.id
  account_name = "mystorageacc"
  account_key  = random_string.account_key.result
  queue_name   = "myqueue"
  visi_timeout = 30

  stage_state_changed {
    state_filter  = "Completed"
    result_filter = "Succeeded"
  }

}

I will continue to the other comments after you confirmation.

Thanks!

@xuzhang3
Copy link
Collaborator

@rdalbuquerque I prefer to name it in the format of azuredevops_servicehook_[consumer_type]_[publisher_type]

@rdalbuquerque
Copy link
Contributor Author

rdalbuquerque commented Nov 23, 2023

Some acceptance tests are still failing. Debugging it, looks like it's something related to the event_config and the fact that it has to be set as Computed to be optional
I'm not sure if the current design is the best one, but I didn't really like the last one. So currently there is a published_event argument and an event_config block.

resource "azuredevops_servicehook_storage_queue_pipelines" "example" {
  project_id = azuredevops_project.example.id
  account_name = azurerm_storage_account.example.name
  account_key = azurerm_storage_account.example.primary_access_key 
  queue_name = azurerm_storage_queue.example.name
  visi_timeout = 30
  published_event = "RunStateChanged"
  event_config {
    run_state_filter = "Completed"
    run_result_filter = "Succeeded"
  }
}

@rdalbuquerque
Copy link
Contributor Author

Ok, added more acceptance tests and they are passing now. :)

refactored again so it's easier to validate and more intuitive to read imo

resource "azuredevops_servicehook_storage_queue_pipelines" "example" {
  project_id   = azuredevops_project.example.id
  account_name = azurerm_storage_account.example.name
  account_key  = azurerm_storage_account.example.primary_access_key 
  queue_name   = azurerm_storage_queue.example.name
  visi_timeout = 30
  published_event = "RunStateChanged"
  run_state_changed_event {
    run_state_filter = "Completed"
    run_result_filter = "Succeeded"
  }
}

@xuzhang3
Copy link
Collaborator

=== RUN   TestAccServicehookStorageQueuePipelines_basic
=== PAUSE TestAccServicehookStorageQueuePipelines_basic
=== RUN   TestAccServicehookStorageQueuePipelines_Update
=== PAUSE TestAccServicehookStorageQueuePipelines_Update
=== RUN   TestAccServicehookStorageQueuePipelines_accountKeyError
=== PAUSE TestAccServicehookStorageQueuePipelines_accountKeyError
=== RUN   TestAccServicehookStorageQueuePipelines_NoEventConfig_CreateAndUpdate
=== PAUSE TestAccServicehookStorageQueuePipelines_NoEventConfig_CreateAndUpdate
=== RUN   TestAccServicehookStorageQueuePipelines_AddEventConfig
=== PAUSE TestAccServicehookStorageQueuePipelines_AddEventConfig
=== RUN   TestAccServicehookStorageQueuePipelines_RemoveEventConfigAndChangeEvent
=== PAUSE TestAccServicehookStorageQueuePipelines_RemoveEventConfigAndChangeEvent
=== RUN   TestAccServicehookStorageQueuePipelines_RemoveEventConfig
=== PAUSE TestAccServicehookStorageQueuePipelines_RemoveEventConfig
=== CONT  TestAccServicehookStorageQueuePipelines_basic
=== CONT  TestAccServicehookStorageQueuePipelines_AddEventConfig
=== CONT  TestAccServicehookStorageQueuePipelines_RemoveEventConfig
=== CONT  TestAccServicehookStorageQueuePipelines_RemoveEventConfigAndChangeEvent
=== CONT  TestAccServicehookStorageQueuePipelines_accountKeyError
=== CONT  TestAccServicehookStorageQueuePipelines_NoEventConfig_CreateAndUpdate
=== CONT  TestAccServicehookStorageQueuePipelines_Update
--- PASS: TestAccServicehookStorageQueuePipelines_accountKeyError (12.53s)
--- PASS: TestAccServicehookStorageQueuePipelines_basic (74.49s)
--- PASS: TestAccServicehookStorageQueuePipelines_RemoveEventConfigAndChangeEvent (98.84s)
--- PASS: TestAccServicehookStorageQueuePipelines_AddEventConfig (99.08s)
--- PASS: TestAccServicehookStorageQueuePipelines_RemoveEventConfig (104.36s)
--- PASS: TestAccServicehookStorageQueuePipelines_Update (104.51s)
--- PASS: TestAccServicehookStorageQueuePipelines_NoEventConfig_CreateAndUpdate (107.75s)
PASS
ok      github.com/microsoft/terraform-provider-azuredevops/azuredevops/internal/acceptancetests        114.382s

@xuzhang3 xuzhang3 merged commit 8b68757 into microsoft:main Dec 13, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants