ci: use binary of airbyte-ci instead of dev version that requires python10 #208
Conversation
aaronsteers
changed the title
📝 WalkthroughWalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested reviewers
Hey there! I noticed you've made a change to the CI tool installation method. Would you mind sharing a bit more context about why you switched from the development to the binary installation? 🤔 Curious to understand the motivation behind this change. Wdyt? 😊 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/connector-tests.yml (1)
Line range hint
7-11: Thoughts on workflow dependencies? 🤔I see there's a TODO about running this after "PyTest (Fast)". Since we're making changes to the workflow, would this be a good time to implement that dependency? It could help reduce unnecessary runs and save CI resources. wdyt?
Example implementation if you're interested:
on: workflow_run: workflows: ["PyTest (Fast)"] types: - completed branches: - main
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/connector-tests.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (1)
.github/workflows/connector-tests.yml (1)
Line range hint
134-139: Hey there! Quick question about the command consistency 🤔I notice we're installing the binary version with
make tools.airbyte-ci-binary.installbut still using theairbyte-ci-devcommand. Should we be usingairbyte-ciinstead to match the binary installation? wdyt?Let's check if the binary installation target exists and what command it exposes:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/logger.py (1)
Line range hint
85-92: Love the clean implementation! A couple of small suggestions if you're interested? 🤔The method looks great! Really clever use of set operations to isolate the extra fields. Would you consider:
- Adding type hints for the empty LogRecord parameters? Something like:
logging.LogRecord("", 0, "", 0, None, None, None, None) # type: ignore[arg-type]
- Adding a doctest to showcase the usage? It would make it super clear how it works, wdyt?
🧰 Tools
🪛 GitHub Actions: Linters
[warning] File requires formatting. The code does not conform to the project's formatting standards and needs to be reformatted using Ruff.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte_cdk/logger.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linters
airbyte_cdk/logger.py
[warning] File requires formatting. The code does not conform to the project's formatting standards and needs to be reformatted using Ruff.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (2)
airbyte_cdk/logger.py (2)
Line range hint
56-64: Nicely integrated with the existing format method! 👍The way you've integrated the new method with DEBUG level handling while maintaining the secret filtering is really clean. The separation of concerns between regular logs and debug logs is well thought out.
🧰 Tools
🪛 GitHub Actions: Linters
[warning] File requires formatting. The code does not conform to the project's formatting standards and needs to be reformatted using Ruff.
83-83: Hey there! Mind running Ruff on this file? 🛠️The pipeline is flagging that this file needs formatting. Would you like me to help you set up a pre-commit hook to catch these automatically?
✅ Verification successful
Hey! I found two easy ways to fix those formatting issues! 🎨
You can run these locally with Poetry:
poetry run ruff format . poetry run ruff check --fix .Or if you'd prefer, a maintainer can run the
/auto-fixcommand in a PR comment to handle this automatically. Would either of those options work for you? 😊🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if ruff is configured in the project fd -g 'ruff.toml|pyproject.toml' -x cat {} \; | grep -i 'ruff'Length of output: 65
Script:
#!/bin/bash # Search for Ruff configuration in various potential locations echo "=== Searching for config files ===" fd -g '*.toml|*.ini|*.cfg|*.yaml|*.yml' -x grep -l 'ruff' {} \; echo -e "\n=== Checking CI workflows ===" fd -g '*.yml' -t f .github/workflows -x grep -l 'ruff' {} \; echo -e "\n=== Checking requirements files ===" fd -g '*requirements*.txt|setup.py|poetry.lock' -x grep -i 'ruff' {} \;Length of output: 518
Script:
#!/bin/bash echo "=== python_lint.yml content ===" cat .github/workflows/python_lint.yml echo -e "\n=== autofix-command.yml content ===" cat .github/workflows/autofix-command.ymlLength of output: 8135
🧰 Tools
🪛 GitHub Actions: Linters
[warning] File requires formatting. The code does not conform to the project's formatting standards and needs to be reformatted using Ruff.
|
It looks like this is working now. Once I see the tests are passed, I'll run |
|
Confirmed. These all worked as expected. The only failing test is the format check, which I'll fix now with After that, this should auto-merge.
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
/autofix
|
Summary by CodeRabbit
AirbyteLogFormatter class