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

refactor(logging): replace error! by info! in the actions of format and remove #143

Merged
merged 2 commits into from
Apr 23, 2024

Conversation

YuanYuYuan
Copy link
Contributor

@YuanYuYuan YuanYuYuan commented Apr 22, 2024

Hi there! I found that a log containing errors can stop the Github action. However, the actions format and remove should not be treated as errors since they're intended by the users.

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

We exit with non-zero code, so IMO use error doesn't hurt.

If we think of it's intended by users, info is more proper.

@YuanYuYuan
Copy link
Contributor Author

We exit with non-zero code, so IMO use error doesn't hurt.

If we think of it's intended by users, info is more proper.

Hi @tisonkun! This is the failed log we encountered. I also have no idea why it failed directly. Regarding the logging level, I agree with your point. I'll make the change.

@YuanYuYuan YuanYuYuan changed the title refactor(logging): replace error! by warn! in the actions of format and remove refactor(logging): replace error! by info! in the actions of format and remove Apr 22, 2024
@tisonkun
Copy link
Member

@YuanYuYuan The workflow fails because we exit with -1, unrelated to these logs.

You can run the commit change step with if: failure() condition, see: https://docs.github.com/en/actions/learn-github-actions/expressions#failure.

It also helps to ensure that you only commit if any file gets changed.

@tisonkun
Copy link
Member

I tend to keep the current code as is then.

@YuanYuYuan
Copy link
Contributor Author

YuanYuYuan commented Apr 22, 2024

@YuanYuYuan The workflow fails because we exit with -1, unrelated to these logs.

You can run the commit change step with if: failure() condition, see: https://docs.github.com/en/actions/learn-github-actions/expressions#failure.

It also helps to ensure that you only commit if any file gets changed.

Yes. Similarly, we used continue_on_error to workaround it. But I'm wondering if this is a good design. Let me explain the reasons.

  1. I didn't expect to receive errors when running hawkeye format to format the files we want to change. And the unknown files are complained with warn instead. Referring to one of the popular tools, SkyWalking Eyes use INFO to inform the users.
  2. An auto fix integrated with the Github action is quite appealing. Just like the example in SkyWalking Eyes. As hawkeye format has already been able to return 1 to indicate any missing headers, do you think it's good to change the return code in hawkeye format and hawkeye remove so that users no longer need to use the above tricks?

@tisonkun
Copy link
Member

@YuanYuYuan Make sense. Let me update the exit code part and release a v6.0.

I'll ping you as a reviewer when a patch is ready.

@tisonkun tisonkun merged commit fc8f24f into korandoru:main Apr 23, 2024
15 checks passed
@YuanYuYuan
Copy link
Contributor Author

@YuanYuYuan Make sense. Let me update the exit code part and release a v6.0.

I'll ping you as a reviewer when a patch is ready.

Many thanks! 😃

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.

2 participants