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

Index receivers using webhook path as key #506

Merged
merged 1 commit into from
May 3, 2023
Merged

Conversation

aryan9600
Copy link
Member

@aryan9600 aryan9600 commented Apr 17, 2023

Use .status.webhookPath as a key to index Receivers. Use this key while listing Receivers during the handling of a payload.
This enables us to list only the Receivers required instead of listing all the Receivers in the entire cluster.

Copy link
Member

@makkes makkes left a comment

Choose a reason for hiding this comment

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

Would you mind adding some more information about why this change is necessary to the PR description, please?

@aryan9600 aryan9600 marked this pull request as draft April 17, 2023 17:29
@aryan9600 aryan9600 marked this pull request as ready for review April 17, 2023 18:40
internal/controllers/receiver_controller.go Outdated Show resolved Hide resolved
internal/controllers/suite_test.go Outdated Show resolved Hide resolved
internal/server/receiver_handler_test.go Outdated Show resolved Hide resolved
@darkowlzz darkowlzz added the area/receiver Webhook receiver related issues and PRs label Apr 21, 2023
Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

Did some testing to see if the index gets updated when the token changes and the index gets deleted when the receiver is deleted, and it does. Querying for the old index based webhook path results in 404.

Overall, it looks good to me.

Left one last minor comment.

internal/server/receiver_handlers.go Outdated Show resolved Hide resolved
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @aryan9600 🏅

Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

Much less computing and more precise reporting about endpoint availability in minimal LOC of changes, absolute great work @aryan9600 🥇

Use `.status.webhookPath` as a key to index Receivers. Use this key
while listing Receivers during the handling of a payload.

Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

Thanks for putting the icing on the 🍰

@hiddeco hiddeco merged commit 4effb15 into fluxcd:main May 3, 2023
@stefanprodan stefanprodan changed the title index receivers using webhook path as key Index receivers using webhook path as key May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/receiver Webhook receiver related issues and PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants