-
Notifications
You must be signed in to change notification settings - Fork 48
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
Do not show build status in manual test checks #2375
Do not show build status in manual test checks #2375
Conversation
Build failed. ✔️ pre-commit SUCCESS in 2m 21s |
@@ -652,6 +652,9 @@ def report_status_to_all_test_jobs( | |||
for test_job in self.job_tests_all: | |||
if test_job.skip_build: | |||
continue | |||
elif test_job.manual_trigger: | |||
if state not in (BaseCommitStatus.error, BaseCommitStatus.failure): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that showing errors can make it easier for a user to understand why tests are not running, but then I realized that when the copr build will work the failed manual test check will not be overwritten. And this is misleading. So it is probably better never to show these check statuses at all. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather be consistent and really hide all the statuses
c058580
to
d75c794
Compare
Build succeeded. ✔️ pre-commit SUCCESS in 2m 27s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
otherwise looks good 👍
@@ -652,6 +652,9 @@ def report_status_to_all_test_jobs( | |||
for test_job in self.job_tests_all: | |||
if test_job.skip_build: | |||
continue | |||
elif test_job.manual_trigger: | |||
if state not in (BaseCommitStatus.error, BaseCommitStatus.failure): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather be consistent and really hide all the statuses
d75c794
to
40f53f9
Compare
Build failed. ✔️ pre-commit SUCCESS in 2m 38s |
This can be improved having a real dependency between tests and builds. Showing a neutral state for a successful build message is not working on gitlab and even on github is not always working (my test falls back on commit statuses and they don't have a neutral state). I can not show only errors related with build statuses because they will be not cleaned up by successful statuses later and this is misleading. We can just hide them all, at least until we have real dependencies.
40f53f9
to
ce2a788
Compare
Build succeeded. ✔️ pre-commit SUCCESS in 2m 15s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
Build succeeded (gate pipeline). ✔️ pre-commit SUCCESS in 2m 18s |
0a85e3c
into
packit:main
Unless there are failures or errors.
This can be improved having a real dependency
between tests and builds.
Showing a neutral state for a successful build
message is not working on gitlab and either on
github is not working always
(my test falls back on commit statuses and
they don't have a neutral state).
Fixes #2147
RELEASE NOTES BEGIN
Packit no longer shows status checks for not yet triggered manual tests.
RELEASE NOTES END