-
-
Notifications
You must be signed in to change notification settings - Fork 62
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
fix: report failure when at least one of the target is failing #638
Conversation
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.
LGTM but I defer to @olblak because it might break some use cases that we did not htink about.
Co-authored-by: Olivier Vernin <olivier@vernin.me>
If I remember correctly, the motivation to fail early after one failing target was to save some time (api call) but I think your suggestion improves the user experience as we see immediately what target is succeeding and those failing. |
Correct ! Thanks for the memory. If I'm not mistaken, it was beforr you reworked the scm to have pullrequest/scm as top level: the proposal here makes sense now (while it was hard to implement before the rework). |
Co-authored-by: Olivier Vernin <olivier@vernin.me>
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.
LGTM (under the assumption that manual testing has to be done)
I'll do some test at the end of the day |
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.
There is something fishy/
I tested this PR using
targets:
1:
kind: shell
spec:
command: "true"
2:
kind: shell
spec:
command: "false"
3:
kind: shell
spec:
command: "false"
4:
kind: shell
spec:
command: "false"
and I got
REPORTS:
✗ TEST.YAML:
Sources:
Target:
✔ [1] (kind: shell)
- [2] (kind: shell)
- [3] (kind: shell)
- [4] (kind: shell)
where it stopped after one target failure
@lemeurherve I am opening a pullrequest on this one with a suggestion |
Signed-off-by: Olblak <me@olblak.com>
Oops sorry I pushed directly to your branch :/, feel free to review my commit |
With the manifest that I mention earlier, I get the following output
|
Signed-off-by: Olblak <me@olblak.com>
Reviewing your commits, thanks @olblak! |
@olblak LGTM 😃 |
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 took the opportunity to fix minor issue
- Always run all target no matter if an issue occur or not
- fix small output issue
Please note that we don't handle the case where a target depends on another target which already failed |
I am planning to implement the missing depends on ccheck |
Signed-off-by: Olblak <me@olblak.com>
…li into is632-report-failure
output
|
Signed-off-by: Olblak <me@olblak.com>
I am waiting for a final approval before merging this PR |
After some test, I didn't catch any regression |
Thanks @lemeurherve |
fix: report failure when at least one of the target is failing
Closes #632
Test
To test this pull request, you can run the following commands:
Additional Information
Tradeoff
Potential improvement