Skip to content

fix(bcos-action): replace print() with logging module (#6229)#6304

Open
crowniteto wants to merge 2 commits into
Scottcjn:mainfrom
crowniteto:fix/anchor-logging-and-readme-pdf
Open

fix(bcos-action): replace print() with logging module (#6229)#6304
crowniteto wants to merge 2 commits into
Scottcjn:mainfrom
crowniteto:fix/anchor-logging-and-readme-pdf

Conversation

@crowniteto
Copy link
Copy Markdown

Summary

Fixes #6229 - Replace print() with logging module in .github/actions/bcos-action/anchor.py.

Changes

  • Added import logging and module-level logger instance
  • Replaced all 7 print() calls with appropriate log levels:
    • Missing env vars: logger.warning()
    • Successful anchor: logger.info()
    • Transaction/Block details: logger.info()
    • HTTP error: logger.error()
    • Response body: logger.debug()
    • Node unavailable: logger.warning()
  • Added logging.basicConfig() with structured format at entry point
  • Used %-style formatting for logger calls (Python logging best practice)

Why

Structured logging is essential in CI/CD pipelines for:

  • Filtering by log level (INFO, WARNING, ERROR, DEBUG)
  • Consistent formatting with timestamps and logger names
  • Integration with monitoring and alerting systems
  • Better debugging when anchor operations fail

Testing

Syntax validated with ast.parse(). All replacements maintain the same output semantics.

Replace all print() calls in anchor.py with the logging module for
better control over log levels, formatting, and CI/CD integration.

Changes:
- Add logging import and logger instance
- Replace print() with logger.info/warning/error/debug
- Add logging.basicConfig() with structured format
- Use %-style formatting for logger (best practice)

Fixes Scottcjn#6229
@crowniteto crowniteto requested a review from Scottcjn as a code owner May 25, 2026 08:25
@github-actions github-actions Bot added BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) tests Test suite changes ci size/S PR: 11-50 lines labels May 25, 2026
Copy link
Copy Markdown

@MolhamHamwi MolhamHamwi left a comment

Choose a reason for hiding this comment

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

I reviewed .github/actions/bcos-action/anchor.py and the added tests/test_p2p_blocks.py changes.

Two specific observations:

  1. tests/test_p2p_blocks.py only contains import pytest and is unrelated to the BCOS action logging change. If this file was accidentally carried over from another branch, dropping it would keep the PR focused and avoid adding a no-op test module.

  2. In anchor.py, the HTTP error response body was previously always printed on failure, but now it is logged with logger.debug("Response: %s", error_body) while basicConfig defaults to INFO. In the GitHub Action default path this means useful node/API failure details may disappear from CI logs; consider logger.warning or logger.error for the response body, possibly with truncation if it can be large.

Positive note: replacing f-string/print output with parameterized logger calls for the success path is a good cleanup because it lets the action caller control formatting and verbosity.

I received RTC compensation for this review.

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

LGTM! Great work on this PR. 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) ci size/S PR: 11-50 lines tests Test suite changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Report a Bug: Use logging instead of print() in .github/actions/bcos-action/anchor.py

3 participants