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

PR e2e test: Validate the regex before building the images #4750

Closed
JorTurFer opened this issue Jun 29, 2023 · 3 comments · Fixed by #5783
Closed

PR e2e test: Validate the regex before building the images #4750

JorTurFer opened this issue Jun 29, 2023 · 3 comments · Fixed by #5783
Labels
help wanted Looking for support from community stale-bot-ignore All issues that should not be automatically closed by our stale bot

Comments

@JorTurFer
Copy link
Member

Proposal

Currently, we build the images and deploy them into the cluster before checking the provided regex, this could deal in a failure 20 minutes later.
We should check somehow the regex before starting the process to be more efficient and saving resources

Use-Case

No response

Is this a feature you are interested in implementing yourself?

Yes

Anything else?

No response

@JorTurFer JorTurFer added needs-discussion feature-request All issues for new features that have not been committed to labels Jun 29, 2023
@tomkerkhove tomkerkhove added help wanted Looking for support from community and removed needs-discussion feature-request All issues for new features that have not been committed to labels Jul 5, 2023
@sbdtu5498
Copy link

Hey, @JorTurFer can you give me a bit more context on what regex is being used for here specifically or would be used for?

@JorTurFer
Copy link
Member Author

JorTurFer commented Jul 7, 2023

Hey, sure!
Currently, we have a mechanism for running e2e tests as part of PRs based on allowed member comments (/run-e2e). As we want to allow not executing the whole tests suite, we use a regex that can be provided with the comment (/run-e2e REGEX_HERE).

The problem is that if the regex is wrong and doesn't match with any file, we want that the execution fails because we have introduced something wrong. Where is the problem? When the regex is executed. Basically we build the images, prepare the cluster, deploy pod identities, KEDA, etc, and then we use the regex for getting the test cases. This is slow and could take 20-25 just for failing:
image

e2e test suite is executed using this file through make (make e2e-test). This is the function that applies the regex for filtering files.

In order so save time and resources building the image and deploying things into the cluster, just for detecting the typo in the regex, I thought to extract that function and run them in the 2nd job of the workflow, before building the image. In case of no matches, raising an error and finishing the execution there, having saved 20 minutes.

Why in the 2nd job and not in the 1st, to save more time and resources? Because as we use a comment trigger, every comment wherever in the repo will trigger the workflow, and this regex validation needs to pull the code (comment trigger doesn't checkout the code) and execute the code. In this 1st job, an arbitrary change in the code could expose sensitive data, as everybody can comment in the repo and this validation needs to checkout the PR code for executing that regex. (we should use the PR branch because in other case, changes in the system can't be tested before going main, the only file executed from main should be the workflow yaml)

I know that we can just add a condition in the specific step to not be executed, but the second reason for adding in the 2nd job, is that the second job already has to checkout the code, so in my mind sounds like the better place:
image

About how to implement this? Maybe we could extract the function and execute it using make for example, or just with go run (but personally I prefer to add a make target), If the function returns 0 matches, we should fail the check and notify the output with the emote in the message and failing the CI check (like we do after e2e failure)

@stale
Copy link

stale bot commented Sep 6, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale All issues that are marked as stale due to inactivity label Sep 6, 2023
@JorTurFer JorTurFer added the stale-bot-ignore All issues that should not be automatically closed by our stale bot label Sep 7, 2023
@stale stale bot removed the stale All issues that are marked as stale due to inactivity label Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Looking for support from community stale-bot-ignore All issues that should not be automatically closed by our stale bot
Projects
Status: Ready To Ship
3 participants