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

chore: replace AirbyteLogger with logging.Logger in connectors #40215

Merged
merged 8 commits into from
Jun 27, 2024

Conversation

natikgadzhi
Copy link
Contributor

What

This commit does all the AirbyteLogger replacing in a single pass, without bumping connector versions. Here's why I did this (internal Slack).

The goal here is to take all connectors to be more ready to update, without spending a week on extra hoops. This change seems very safe and tactical.

This closes https://github.com/airbytehq/airbyte-internal-issues/issues/8406.

How

Replaces:

  • from airbyte_cdk import AirbyteLogger -> import logging
  • from airbyte_cdk.logger import AirbyteLogger -> import logging
  • logger: AirbyteLogger -> logger: logging.Logger
  • AirbyteLogger() -> logging.getLogger("airbyte")

And then airbyte-ci format fix all.

User Impact

None, connectors will not get republished.

Can this PR be safely reverted and rolled back?

  • YES 💚
  • NO ❌

Caveats

The CI on this will be very red because I'm changing connector code without bumping the version, so needs manual blessing from @alafanechere, @ChristoGrab, and @girarda.

- from airbyte_cdk import AirbyteLogger -> import logging
- from airbyte_cdk.logger import AirbyteLogger -> import logging
- logger: AirbyteLogger -> logger: logging.Logger
- AirbyteLogger() -> logging.getLogger("airbyte")
- airbyte-ci format fix all
@natikgadzhi natikgadzhi requested a review from a team as a code owner June 22, 2024 14:29
Copy link

vercel bot commented Jun 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Jun 27, 2024 6:32am

@octavia-squidington-iv octavia-squidington-iv requested review from a team June 25, 2024 20:36
@btkcodedev btkcodedev force-pushed the ng/airbyte-logging-rampage branch from 853f2d0 to 73113b7 Compare June 25, 2024 20:41
@octavia-squidington-iii octavia-squidington-iii added area/documentation Improvements or additions to documentation and removed area/documentation Improvements or additions to documentation labels Jun 25, 2024
@btkcodedev
Copy link
Collaborator

Hi, @alafanechere, Can you please review this PR
CC: @natikgadzhi

@natikgadzhi
Copy link
Contributor Author

@btkcodedev well, shit. Our tooling is so cool that there are already too many pull requests that are conflicting with this.

We will have to open a new PR with these changes, bot on top of the fresh master.

@alafanechere before we do so, let's confirm that this PR is OK in spirit. What @btkcodedev will do is:

  1. Replacing:
    from airbyte_cdk import AirbyteLogger -> import logging
    from airbyte_cdk.logger import AirbyteLogger -> import logging
    logger: AirbyteLogger -> logger: logging.Logger
    AirbyteLogger() -> logging.getLogger("airbyte")
  2. airbyte-ci connectors --modified bump-version --patch -m "Refactor deprecated loggers"
  3. airbyte-ci format fix all
  4. Push a single large PR

At that point, it's a good idea to merge asap so we don't rack up new conflicts.

Copy link
Contributor

@alafanechere alafanechere left a comment

Choose a reason for hiding this comment

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

I'd 👍 this PR if it passed CI.
Multiple connectors are failing.
source-bing-ads for instance is failing it's unit tests for reasons which look related to logging.

My suggestion is to:

  • Remove from this branch all connectors not passing CI
  • Then we can safely merge this one once CI is 🟢 (@btkcodedev you can add the auto-merge label to the PR once you've made this change)
  • Open per connector PR for the 🔴 one
  • Manually inspect the failure reason of these 🔴 ones and use /approve-and-merge if they're definitely not related to the logger change.

Tooling wise the following flow can be followed to open per connectors PRs:

  1. Make batch logger changes on all remaining connectors
  2. Run airbyte-ci connectors --modified bump-version patch "replace AirbyteLogger..."
  3. Run airbyte-ci connectors --modified pull-request --message="Replace AirbyteLogger...

@btkcodedev btkcodedev force-pushed the ng/airbyte-logging-rampage branch from 67f01b1 to d5fc3ec Compare June 27, 2024 04:40
@octavia-squidington-iii octavia-squidington-iii removed the area/documentation Improvements or additions to documentation label Jun 27, 2024
@octavia-squidington-iv octavia-squidington-iv requested a review from a team June 27, 2024 04:41
@btkcodedev
Copy link
Collaborator

CI green for this branch

@btkcodedev
Copy link
Collaborator

btkcodedev commented Jun 27, 2024

Linking PRs with failed CI:
#40578
#40579
#40580
#40581
#40582
#40584
#40585
#40586
#40587
#40588
#40589

@alafanechere alafanechere merged commit 30643ae into master Jun 27, 2024
66 of 67 checks passed
@alafanechere alafanechere deleted the ng/airbyte-logging-rampage branch June 27, 2024 16:02
xiaohansong pushed a commit that referenced this pull request Jul 9, 2024
Co-authored-by: btkcodedev <btk.codedev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment