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

pkg/test: allow complex "assert" and "errors" step file names #258

Merged
merged 1 commit into from
Dec 18, 2020

Conversation

estroz
Copy link
Contributor

@estroz estroz commented Dec 16, 2020

What this PR does / why we need it:
This PR improves file name matching regexps by adding meta char escapes and non-capturing groups, and permitts assert- and error-type steps to have complex names, ex. "00-assert-test-pod.yaml"

Signed-off-by: Eric Stroczynski estroczy@redhat.com

@kensipe
Copy link
Member

kensipe commented Dec 16, 2020

nice! thank you sir! I like it. I will review in more detail shortly... but a quick pass looks good!
(shortly being I will complete tonight)

Copy link
Member

@kensipe kensipe left a comment

Choose a reason for hiding this comment

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

/lgtm

❤️ it! thanks @estroz

Copy link
Contributor

@alenkacz alenkacz left a comment

Choose a reason for hiding this comment

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

I am a bit lost here (not your fault, that's the state of the code as it was before you started working on it).

I feel like right now the file parsing logic is two places, but it's parsing the same files, right? The regexes are kind of similar but not equal 🤔 what am I missing here? Can we maybe make some cleanup in there since we're doing changes in that area already?

pkg/test/case.go Outdated Show resolved Hide resolved
pkg/test/case.go Outdated Show resolved Hide resolved
escapes and non-capturing groups, and permitted assert- and error-
type steps to have complex names, ex. "00-assert-test-pod.yaml"

Signed-off-by: Eric Stroczynski <estroczy@redhat.com>
Copy link
Member

@kensipe kensipe left a comment

Choose a reason for hiding this comment

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

/lgtm

@kensipe
Copy link
Member

kensipe commented Dec 17, 2020

looking to see if @alenkacz is good... thanks Eric!

@estroz
Copy link
Contributor Author

estroz commented Dec 17, 2020

@kensipe thanks!

@alenkacz the regexps are very similar and can probably be merged into one with a larger refactoring, however I don't think I have time to do one now. I will try when I have more time!

Copy link
Contributor

@alenkacz alenkacz left a comment

Choose a reason for hiding this comment

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

Thanks

@alenkacz alenkacz merged commit 9447755 into kudobuilder:main Dec 18, 2020
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.

None yet

3 participants