Skip to content

Conversation

@t0mmylam
Copy link
Collaborator

@t0mmylam t0mmylam commented Sep 5, 2025

Summary

  • Add glob support for configInterrupts so patterns with * can target multiple ConfigMap keys.
  • Validation requires each pattern to match at least one key; exact keys still require exact matches.
  • Runtime applies a union of all matching patterns and de-duplicates identical interrupts (same type + services).

Implementation

  • operator/api/v1alpha1/skyhook_webhook.go: Validate configInterrupts keys as exact or * globs using filepath.Match; error if a glob matches no keys.
  • operator/internal/wrapper/skyhook.go: For each changed config key, match against all patterns with filepath.Match; collect all matching interrupts; de-duplicate by type|services; copy interrupt before taking address. Added path/filepath and strings imports.

Test

  • operator/api/v1alpha1/skyhook_webhook_test.go: Added tests that accept a glob which matches at least one key and reject a glob that matches none.

@lockwobr
Copy link
Collaborator

lockwobr commented Sep 5, 2025

Maybe I am wrong, but I thought there was going to be more to this one. Has this been tested? I think we need an e2e test to make sure this is working as expected and we dont end up with a regression. Maybe add to the current tests on this feature a test to cover this.

@t0mmylam
Copy link
Collaborator Author

t0mmylam commented Sep 5, 2025

Maybe I am wrong, but I thought there was going to be more to this one. Has this been tested? I think we need an e2e test to make sure this is working as expected and we dont end up with a regression. Maybe add to the current tests on this feature a test to cover this.

In the ticket it said:

This feature must include unit tests for this logic, not e2e tests.

But i'm happy to add them!

@lockwobr
Copy link
Collaborator

lockwobr commented Sep 8, 2025

Funny, wrote this ticket a long time ago. I think we should update an existing e2e test to cover this. I think I didn't want a new test because they take so long to run.

This feature must include unit tests for this logic, not e2e tests.

But i'm happy to add them!

@t0mmylam t0mmylam merged commit 7ecb54c into main Sep 8, 2025
6 checks passed
@t0mmylam t0mmylam deleted the config-interrupts-glob-support branch September 8, 2025 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants