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

diff/report: fix key regexp #379

Merged
merged 1 commit into from
Apr 24, 2022
Merged

diff/report: fix key regexp #379

merged 1 commit into from
Apr 24, 2022

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