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

include common directory in tests on path changes #260

Conversation

Gregory-Pereira
Copy link
Collaborator

@Gregory-Pereira Gregory-Pereira commented Apr 13, 2024

Fixing tests, will also use this PR to fix any code changes that correspond to failing tests.

Broken out from #258.

Changes:

  • add a check on jobs in the standard development workflows (all apps, model_servers, model_image_build_push) to not run if there is a hold-tests label, I thought this could save us some resources
  • fix codegen's Download model step due changing make target name
  • fix chromedriver and chrome downloads failling due to timeout
    • move binaries to /recipes/common/bin
  • swapped install make target in both recipes/common and each recipe to be a double colon rule rather than a single colon rule, which allows us to run common and local install commands together

@Gregory-Pereira Gregory-Pereira force-pushed the include-common-directories-in-tests-paths branch 7 times, most recently from f8cbaf1 to 2c3b906 Compare April 13, 2024 20:03
@Gregory-Pereira
Copy link
Collaborator Author

Tests pass, going to add the hold-tests label and push an empty commit to see if it works.

@Gregory-Pereira Gregory-Pereira added the hold-tests This label holds CI workflows label Apr 13, 2024
@Gregory-Pereira
Copy link
Collaborator Author

Can confirm that the labels to skip tests work. Removing hold-tests label

@Gregory-Pereira Gregory-Pereira removed the hold-tests This label holds CI workflows label Apr 13, 2024
@Gregory-Pereira
Copy link
Collaborator Author

/retest

empty commit testing `hold-tests` label succeeds, squashing

Signed-off-by: greg pereira <[email protected]>
@Gregory-Pereira Gregory-Pereira force-pushed the include-common-directories-in-tests-paths branch from eab60c4 to 42d30ea Compare April 14, 2024 02:57
@@ -22,6 +24,7 @@ env:

jobs:
build-and-push-image:
if: "!contains(github.event.pull_request.labels.*.name, 'hold-tests')"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure folks would remember to use this trick

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was going to have a discussion with the team about it. I know I would use it because sometime I just want to see the diff for review purposes and dont want to waist the resources. If your worried about visibility maybe we could tag every PR with hold-tests to begin with and then use a /ok-to-test similar to prow?

Copy link
Collaborator

@lmilbaum lmilbaum left a comment

Choose a reason for hiding this comment

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

LGTM

@Gregory-Pereira Gregory-Pereira merged commit f42a8fc into containers:main Apr 14, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants