-
Notifications
You must be signed in to change notification settings - Fork 71
feat(integrations): Add Sonar connector support with git submodule #873
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
base: main
Are you sure you want to change the base?
Changes from all commits
ac23073
c85fae9
1b2883d
930434c
b86455e
06c07d5
8513158
313f142
1361ab1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📝 [actionlint] reported by reviewdog 🐶
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📝 [actionlint] reported by reviewdog 🐶
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📝 [actionlint] reported by reviewdog 🐶
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,8 +25,19 @@ jobs: | |
| runs-on: ubuntu-latest | ||
| steps: | ||
| # Common steps: | ||
| - name: Authenticate as GitHub App | ||
| uses: actions/create-github-app-token@67018539274d69449ef7c02e8e71183d1719ab42 # v2.1.4 | ||
| id: get-app-token | ||
| with: | ||
| owner: "airbytehq" | ||
| repositories: "PyAirbyte,sonar" | ||
| app-id: ${{ secrets.OCTAVIA_BOT_APP_ID }} | ||
| private-key: ${{ secrets.OCTAVIA_BOT_PRIVATE_KEY }} | ||
| - name: Checkout code | ||
| uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 | ||
| with: | ||
| token: ${{ steps.get-app-token.outputs.token }} | ||
| submodules: recursive | ||
|
Comment on lines
+28
to
+40
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainVerify GitHub App token is available and properly scoped for fork PRs. The GitHub App authentication pattern looks correct for accessing the private sonar submodule, but the Do you want to add a conditional check (similar to the I'll verify this concern by examining the workflow file and checking how the GitHub App token action handles missing secrets. 🏁 Script executed: #!/bin/bash
# Inspect the full workflow file to understand pytest-no-creds job config
# and check for existing fork protection conditionals
cat .github/workflows/python_pytest.yml | head -n 350Length of output: 10302 🏁 Script executed: #!/bin/bash
# Also check if there are any conditional checks (if:) in the workflow
rg -n "if:" .github/workflows/python_pytest.yml -C2Length of output: 2982 🏁 Script executed: #!/bin/bash
# Search for pytest-no-creds job definition
rg -n "pytest-no-creds" .github/workflows/python_pytest.yml -A15Length of output: 1924 🌐 Web query: 💡 Result: Short answer: actions/create-github-app-token needs the app private key (a secret) to create an installation token — GitHub will NOT provide repository secrets to workflows triggered by a pull request from a fork, so the action will be unable to read the PRIVATE_KEY (and will fail or not produce a usable token) when the workflow runs on a fork PR. [1][2] What to do (brief):
Sources:
References: Citations:
Add fork protection conditionals to pytest-fast, pytest-no-creds, and dependency-analysis jobs. Your concern is spot-on—but the issue affects more than pytest-no-creds. GitHub does NOT provide repository secrets to workflows triggered by a pull request from a fork, so all three jobs that use This is a design contradiction: the workflow comments explicitly state that pytest-no-creds exists to run tests "on forks, where secrets are not available," yet it now requires secrets for GitHub App authentication. The pytest job correctly protects against this with a fork conditional at lines 170–172, but pytest-fast, pytest-no-creds, and dependency-analysis lack this protection. You'll want to add the same conditional to those three jobs: if: >
(github.event_name == 'push' && github.ref == 'refs/heads/main') ||
(github.event.pull_request.head.repo.fork == false)Wdyt? Should pytest-no-creds perhaps have different logic if it's meant to be fork-friendly, or is the intent to require these jobs to only run on the main repo going forward? 🤖 Prompt for AI Agents |
||
| - name: Set up Poetry | ||
| uses: Gr1N/setup-poetry@48b0f77c8c1b1b19cb962f0f00dff7b4be8f81ec # v9 | ||
| with: | ||
|
|
@@ -90,8 +101,19 @@ jobs: | |
| runs-on: ubuntu-latest | ||
| steps: | ||
| # Common steps: | ||
| - name: Authenticate as GitHub App | ||
| uses: actions/create-github-app-token@67018539274d69449ef7c02e8e71183d1719ab42 # v2.1.4 | ||
| id: get-app-token | ||
| with: | ||
| owner: "airbytehq" | ||
| repositories: "PyAirbyte,sonar" | ||
| app-id: ${{ secrets.OCTAVIA_BOT_APP_ID }} | ||
| private-key: ${{ secrets.OCTAVIA_BOT_PRIVATE_KEY }} | ||
| - name: Checkout code | ||
| uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 | ||
| with: | ||
| token: ${{ steps.get-app-token.outputs.token }} | ||
| submodules: recursive | ||
| - name: Set up Poetry | ||
| uses: Gr1N/setup-poetry@48b0f77c8c1b1b19cb962f0f00dff7b4be8f81ec # v9 | ||
| with: | ||
|
|
@@ -168,8 +190,19 @@ jobs: | |
| PYTHONIOENCODING: utf-8 | ||
| steps: | ||
| # Common steps: | ||
| - name: Authenticate as GitHub App | ||
| uses: actions/create-github-app-token@67018539274d69449ef7c02e8e71183d1719ab42 # v2.1.4 | ||
| id: get-app-token | ||
| with: | ||
| owner: "airbytehq" | ||
| repositories: "PyAirbyte,sonar" | ||
| app-id: ${{ secrets.OCTAVIA_BOT_APP_ID }} | ||
| private-key: ${{ secrets.OCTAVIA_BOT_PRIVATE_KEY }} | ||
| - name: Checkout code | ||
| uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 | ||
| with: | ||
| token: ${{ steps.get-app-token.outputs.token }} | ||
| submodules: recursive | ||
| - name: Set up Python | ||
| uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 | ||
| with: | ||
|
|
@@ -241,8 +274,19 @@ jobs: | |
| name: Dependency Analysis with Deptry | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Authenticate as GitHub App | ||
| uses: actions/create-github-app-token@67018539274d69449ef7c02e8e71183d1719ab42 # v2.1.4 | ||
| id: get-app-token | ||
| with: | ||
| owner: "airbytehq" | ||
| repositories: "PyAirbyte,sonar" | ||
| app-id: ${{ secrets.OCTAVIA_BOT_APP_ID }} | ||
| private-key: ${{ secrets.OCTAVIA_BOT_PRIVATE_KEY }} | ||
| - name: Checkout code | ||
| uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 | ||
| with: | ||
| token: ${{ steps.get-app-token.outputs.token }} | ||
| submodules: recursive | ||
| - name: Set up Python | ||
| uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 | ||
| with: | ||
|
|
@@ -256,5 +300,4 @@ jobs: | |
|
|
||
| # Job-specific step(s): | ||
| - name: Run Deptry | ||
| run: | | ||
| poetry run deptry . | ||
| run: poetry run poe check-deps | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| [submodule "sonar"] | ||
| path = sonar | ||
| url = https://github.com/airbytehq/sonar.git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 [actionlint] reported by reviewdog 🐶
shellcheck reported issue in this script: SC2086:info:2:27: Double quote to prevent globbing and word splitting [shellcheck]
PyAirbyte/.github/workflows/fix-pr-command.yml
Line 102 in 930434c