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: show bacalhau id error #192

Merged
merged 3 commits into from
Jun 27, 2024
Merged

fix: show bacalhau id error #192

merged 3 commits into from
Jun 27, 2024

Conversation

hunjixin
Copy link
Contributor

@hunjixin hunjixin commented Jun 26, 2024

Review Type Requested (choose one):

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

show error in "bacalhau id" command.

Summary

Provide a one line summary and link to any relevant references

Task/Issue reference

Re: https://github.com/Lilypad-Tech/hni/issues/7

Details (optional)

Add any additional details that might help Code Reviewers digest this PR

How to test this code? (optional)

Anything else? (optional)

@github-actions github-actions bot added the fix label Jun 26, 2024
@hunjixin hunjixin requested a review from AquiGorka June 26, 2024 12:49
@taoshengshi taoshengshi requested review from arsen3d and noryev June 26, 2024 12:52
Copy link
Contributor

@AquiGorka AquiGorka left a comment

Choose a reason for hiding this comment

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

@hunjixin thank you for the changes, can you provide a video recording of how this changes affect the POW process?

@hunjixin
Copy link
Contributor Author

bacalhau id have error
image

and when runing rp, lilypad not print pow worker message
image

this pr return this error, and tell user you got sometheing error
image

@AquiGorka

@hunjixin hunjixin force-pushed the fix/show_bacalhau_id_error branch 2 times, most recently from 7a802d0 to 402da7b Compare June 27, 2024 11:28
@hunjixin
Copy link
Contributor Author

hunjixin commented Jun 27, 2024

image

add a new commit to resolve this problem, cmd output some odd error log. just parse the last line @AquiGorka

if err != nil {
return "", fmt.Errorf("error calling get id results %s", err.Error())
}

splitOutputs := strings.Split(strings.Trim(string(runOutputRaw), " \t\n"), "\n")
runOutput := splitOutputs[len(splitOutputs)-1]
Copy link
Contributor

Choose a reason for hiding this comment

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

errOutput = splitOutputs[1:]
if errOutput != nil {
  return "", fmt.Errorf("error unmarshalling job JSON %s %s", err.Error(), errOutput)
}

Copy link
Contributor Author

@hunjixin hunjixin Jun 27, 2024

Choose a reason for hiding this comment

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

image
add like this?, but i got a panic in this change. i dont know how the error log look like

Copy link
Contributor

Choose a reason for hiding this comment

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

If you are getting errors, it means there is something wrong, you also need to trim, I added a similar change recently, here: https://github.com/Lilypad-Tech/lilypad/pull/175/files
Let's make sure we also capture errors if there are errors

Copy link
Collaborator

Choose a reason for hiding this comment

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

@AquiGorka I've updated to add logging for error output.
@hunjixin Looks like we want splitOutputs[:len(splitOutputs)-1] (i.e. everything but the last line).

Copy link
Contributor Author

@hunjixin hunjixin Jun 28, 2024

Choose a reason for hiding this comment

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

this change seem to make things not work.the output in my comment
1 line: error log
1 line: id result
1 line: empty line

this is why add a trim to make sure the empty line is removed

split by \n, 4 lines, with 2 empty line in last

@hunjixin hunjixin force-pushed the fix/show_bacalhau_id_error branch from 402da7b to 91a12b4 Compare June 27, 2024 12:53
@walkah walkah force-pushed the fix/show_bacalhau_id_error branch from 91a12b4 to 8e82923 Compare June 27, 2024 20:14
@walkah walkah requested a review from AquiGorka June 27, 2024 20:14
@walkah
Copy link
Collaborator

walkah commented Jun 27, 2024

In discussing with @taoshengshi - if we can get this merged, he can do testing in the morning (china time) with RPs to confirm this unblocks the issue.

@walkah walkah merged commit d776217 into main Jun 27, 2024
3 checks passed
@walkah walkah deleted the fix/show_bacalhau_id_error branch June 27, 2024 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants