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

feat: add deployment URL mapping to webhook plugin #8000

Merged
merged 1 commit into from
Sep 19, 2024
Merged

Conversation

ktoublanc
Copy link
Contributor

@ktoublanc ktoublanc commented Sep 4, 2024

Summary

This PR add mapping for deployment URL when creating a deployment through web hook plugin.

I'm planning a second merge request on the documentation repository if accepted

I did not see any relevant tests for the web hook plugin but I am willing to contribute as to this as I'm planning to propose other changes (a fix on the deployment name computation which can exceed 255 chars when multiple commits are present and the ability to delete issues created from the web hook)

Does this close any open issues?

No open issues for this change

@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Sep 4, 2024
@dosubot dosubot bot added component/config-ui This issue or PR relates to config-ui component/plugins This issue or PR relates to plugins pr-type/feature-development This PR is to develop a new feature labels Sep 4, 2024
@ktoublanc ktoublanc force-pushed the main branch 2 times, most recently from f5429a8 to 1e27e3d Compare September 10, 2024 09:27
@klesh
Copy link
Contributor

klesh commented Sep 11, 2024

LGTM.
Don't forget to write a migration script to add the column to the database.

@ktoublanc
Copy link
Contributor Author

LGTM. Don't forget to write a migration script to add the column to the database.

Hey, thanks for the review.

I don't get what migration script I should write...
From my point of view, I'm only adding a field in the Webhook API which will value the Url field of the deployment model.
This field is already created in the database (cicd_deployments.url) which is nullable and I can't set a default value for the field and there are no associated tool_webhook... table associated.

Am I getting it right ?

@ktoublanc
Copy link
Contributor Author

@klesh did you have time to check my last comment ?

@klesh
Copy link
Contributor

klesh commented Sep 19, 2024

@ktoublanc You're absolutely right. I wasn't aware that the field already existed—thank you for the clarification!

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Sep 19, 2024
@klesh klesh merged commit aa01188 into apache:main Sep 19, 2024
11 checks passed
@klesh
Copy link
Contributor

klesh commented Sep 19, 2024

Merged, thanks for your contribution. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/config-ui This issue or PR relates to config-ui component/plugins This issue or PR relates to plugins lgtm This PR has been approved by a maintainer pr-type/feature-development This PR is to develop a new feature size:XS This PR changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants