Skip to content

Conversation

eonpatapon
Copy link
Contributor

The current regexp doesn't match all possible apis, for example
cert-manager.io.

As there is a closing ) we can simply take everything before it.

Also resource names can have . in their name.

@eonpatapon
Copy link
Contributor Author

#261
#290

@mumoshu
Copy link
Collaborator

mumoshu commented Mar 2, 2022

@eonpatapon Hey! Thanks a lot for your contribution. I'd appreciate it if you could add some unit tests to cover several possible and varying label keys, and a few impossible/invalid label keys.

The current regexp doesn't match all possible apis, for example
`cert-manager.io`.

As there is a closing `)` we can simply take everything before it.

Also resource names can have . in their name.
@eonpatapon
Copy link
Contributor Author

@mumoshu I've added a test with some valid resources (some of which were problematic)

Copy link
Collaborator

@mumoshu mumoshu left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you so much for fixing it @eonpatapon!

@mumoshu mumoshu merged commit 1a601ac into databus23:master Apr 24, 2022
@eonpatapon eonpatapon deleted the fix-regexp branch June 13, 2022 13:14
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.

2 participants