Skip to content

Conversation

@mieliespoor
Copy link
Contributor

@mieliespoor mieliespoor commented Oct 13, 2025

⚠️ Pre Checklist

Please complete ALL items in this checklist, and remove before submitting

  • I have read through the Contributing Documentation.
  • I have added relevant tests.
  • I have added relevant documentation.
  • I will add labels to the PR, such as pr-type/bug-fix, pr-type/feature-development, etc.

Summary

Add a new endpoint for posting to a webhook using the project name. This will allow for a generic webhook to be created and added to multiple projects, and the webhook retrieved using the project name.

Does this close any open issues?

No

Screenshots

Include any relevant screenshots here.

Other Information

relates to discussion: #6348

@mieliespoor
Copy link
Contributor Author

There are no unit tests for this code and no documentation in this repository related to this.

@mieliespoor mieliespoor marked this pull request as ready for review October 14, 2025 08:20
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. component/plugins This issue or PR relates to plugins pr-type/feature-development This PR is to develop a new feature labels Oct 14, 2025
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Oct 19, 2025
@mieliespoor
Copy link
Contributor Author

mieliespoor commented Oct 20, 2025

Testing this, we are able to push deployments and see these deployment data in the cicd_deployments and cicd_deployment_commits tables. Deployment data from webhooks, from what I can see is not present on many dashboards, but one dashboard in particular has an issue.

Because we now use one single webhook, which is then added to multiple projects, the query for the DORA Details - Deployment Frequency dashboard, specifically the Deployment list in the selected project(s) panel is incorrectly attributing deployments to projects.

Looking into it, we can see that the project_mapping table has the cicd_scope row for the same webook id for the two projects we used to test. This is still expected, because we will be reusing the webhook.

Looking at this part of the query:

with _deployment_commit_rank as(
  SELECT
  	pm.project_name,
  	IF(cdc._raw_data_table != '', cdc._raw_data_table, cdc.cicd_scope_id) as _raw_data_table,
  	cdc.id,
    cdc.display_title,
    cdc.url,
  	cdc.cicd_deployment_id,
  	cdc.cicd_scope_id,
  	result,
  	environment,
    finished_date,
    row_number() over(partition by cdc.cicd_deployment_id order by finished_date desc) as _deployment_commit_rank
  FROM cicd_deployment_commits cdc
  left join project_mapping pm on cdc.cicd_scope_id = pm.row_id and pm.`table` = 'cicd_scopes'
  WHERE
    pm.project_name in ($project)
  	and result = 'SUCCESS'
  	and environment = 'PRODUCTION'
)

I believe the issue to be around the line where the row_number() is determined. Because two rows, but for different projects exist, it does not treat these as distinctly different. What I did try and do, was add pm.project_name to the partition statement. This resulted in both projects showing up, but one of them shouldn't as it is not the project with the deployment data.

I'm not a db dev, nor a dba, so I'm out of my depth to try and solve this issue. Any suggestions here, would be welcomed.

@klesh
Copy link
Contributor

klesh commented Oct 21, 2025

Okay, based on the discussion.
I think the missing part is to "create the missing webhook connection" because the current implementation merely finds an existing connection, then calls PostDeploymentCicdTask.

  1. Check if there is a webhook connection existing for the project.
  2. Yes: call PostDeploymentCicdTask with it
  3. No: create one, and update project/project to include it. then call PostDeploymentCicdTask with it.

postPipelineDeployTaskEndpoint: connection.postPipelineDeployTaskEndpoint,
postPullRequestsEndpoint: connection.postPullRequestsEndpoint,
apiKeyId: connection.apiKey.id,
apiKeyId: connection.apiKey?.id,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

apiKey is optional for a webhook, so we won't always have an apiKey id.

connection := &models.WebhookConnection{}
projectName := input.Params["projectName"]
webhookName := fmt.Sprintf("%s_deployments", projectName)
err := findByProjectName(connection, input.Params, pluginName)
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to pass the webhookName to the findByProjectName function instead of redefining.

}
// mount all api resources for all plugins
for pluginName, apiResources := range resources {
if pluginName == "webhook" {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not the way to handle route conflict.
What was the conflict exactly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not exactly a conflict, but more a duplicate route not wanted. The way routing is done, this route, because how it is defined on the plugin, would have resulted in the route /plugins/webhook/projects/:projectName/deployments instead of only /projects/:projectName/deployments. With this we now only have the latter as we remove the registration of the route on the webhook plugin route. Best way I could think of achieving this, unless the code is moved elsewhere and not in the plugin itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/plugins This issue or PR relates to plugins pr-type/feature-development This PR is to develop a new feature size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants