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

fix: check combined outputs for potential errors when getting bacalhau job id #175

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

AquiGorka
Copy link
Contributor

Review Type Requested (choose one):

  • Glance - superficial check (from domain experts)
  • Logic - thorough check (from everybody doing review)

Summary

These changes check the combined output from bacalhau create --id-only --wait path/to/job/spec for errors because I noticed that sometimes when the id was returned, the rest of the output also contained an error and the code was not checking for it, the rest of the process would execute but there would be no results in the output folder.

How to test this code? (optional)

I don't think I have a straightforward way to reproduce the error, I noticed the situation when testing this PR, I added a print statement to the runOutput var and I noticed the full output also included info to the lines of could not execute job, not enough nodes (I'm writing this from memory so if you are testing this bear in mind it may be a bit different) and the whole cli run was executed but the output folder did not include any files.
With these changes the process did return an error in that situation and the RP console showed the error had happened.

Copy link
Collaborator

@walkah walkah left a comment

Choose a reason for hiding this comment

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

🚢 Looks good to me! If it's tricky to reproduce, perhaps worth a unit test to verify? (obviously that won't catch if bacalhau changes the output formatting...)

@AquiGorka
Copy link
Contributor Author

If it's tricky to reproduce, perhaps worth a unit test to verify?

That's a great suggestion, tracking here because I need to focus on another task but I do want to get my hands dirty with unit tests - we have been relying on existing tests and have not written news tests ourselves.

obviously that won't catch if bacalhau changes the output formatting

We are pinning [email protected] for now because their next release will bring breaking changes.

@AquiGorka AquiGorka merged commit 9164626 into main Jun 20, 2024
@AquiGorka AquiGorka deleted the gorka/fix-run-output-combined-output-with-error branch June 20, 2024 16:22
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

2 participants