fix: skip cloud tests for dependabot when secrets unavailable#28196
fix: skip cloud tests for dependabot when secrets unavailable#28196Narwhal-fish wants to merge 4 commits into
Conversation
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
1 similar comment
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
ef3d04f to
945c24f
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
1 similar comment
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
76fd8cc to
945c24f
Compare
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
e548b80 to
53964d8
Compare
There was a problem hiding this comment.
Pull request overview
Adjusts CI workflow gating so Dependabot-originated runs don’t attempt to execute cloud integration tests when required secrets are unavailable, preventing credential-loading failures in GitHub Actions.
Changes:
- Updates multiple cloud-test step
if:conditions to avoid enabling tests based solely on repo vars when the actor is Dependabot. - Updates PT matrix exclusion override for the Azure suite to avoid enabling it based on vars for Dependabot runs.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
945c24f to
809ac23
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| contains(matrix.modules, 'trino-redshift') && | ||
| (contains(matrix.profile, 'cloud-tests') || contains(matrix.profile, 'fte-tests')) && | ||
| (env.CI_SKIP_SECRETS_PRESENCE_CHECKS != '' || env.AWS_ACCESS_KEY_ID != '' || env.REDSHIFT_SUBNET_GROUP_NAME != '') | ||
| (env.CI_SKIP_SECRETS_PRESENCE_CHECKS != '' || ((env.AWS_ACCESS_KEY_ID != '' || env.REDSHIFT_SUBNET_GROUP_NAME != '') && env.DEPENDABOT_PR != 'true')) |
There was a problem hiding this comment.
The updated Redshift gating skips the step for dependabot PRs even when the run does have secrets (e.g., a repository_dispatch / “test-with-secrets-command” run). Unlike the other AWS-backed steps, this condition no longer has an explicit “secret present” escape hatch, so dependabot PRs can never run Redshift tests even when AWS_SECRET_ACCESS_KEY is available. Adjust the if: to allow execution when the required secret is present (consistent with the (has-var && !dependabot) || has-secret intent).
| (env.CI_SKIP_SECRETS_PRESENCE_CHECKS != '' || ((env.AWS_ACCESS_KEY_ID != '' || env.REDSHIFT_SUBNET_GROUP_NAME != '') && env.DEPENDABOT_PR != 'true')) | |
| (env.CI_SKIP_SECRETS_PRESENCE_CHECKS != '' || | |
| ((env.AWS_ACCESS_KEY_ID != '' || env.REDSHIFT_SUBNET_GROUP_NAME != '') && env.DEPENDABOT_PR != 'true') || | |
| env.AWS_SECRET_ACCESS_KEY != '') |
| CI_SKIP_SECRETS_PRESENCE_CHECKS: ${{ secrets.CI_SKIP_SECRETS_PRESENCE_CHECKS }} | ||
| SECRETS_PRESENT: ${{ secrets.SECRETS_PRESENT }} | ||
| DEPENDABOT_PR: ${{ github.event.pull_request.user.login == 'dependabot[bot]' || github.event.client_payload.pull_request.user.login == 'dependabot[bot]' }} | ||
| PTL_TMP_DOWNLOAD_PATH: /tmp/pt_java_downloads |
There was a problem hiding this comment.
The new dependabot gating is only applied to some cloud-test steps. There are still steps that gate execution on repo vars (which Dependabot can read) while consuming secrets (which Dependabot can’t), so dependabot-originated runs can still execute and then fail when secrets are empty. Concrete examples in this workflow: the “Run Hive AWS Tests” step runs when AWS_ACCESS_KEY_ID var is set, and “Cloud Snowflake Tests” runs when SNOWFLAKE_URL var is set. Apply the same DEPENDABOT_PR guard (or check required secrets directly) in those places too so this change fully fixes dependabot builds.
|
This pull request has gone a while without any activity. Ask for help on #core-dev on Trino slack. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThe PR introduces detection for Dependabot-initiated builds by adding a workflow-level Assessment against linked issues
Out-of-scope changes
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 771-774: The conditional gating the trino-redshift cloud tests
dropped the AWS secret check and therefore skips Redshift when only
REDSHIFT_AWS_SECRET_ACCESS_KEY is provided (e.g., Dependabot). Update the if
expression that checks matrix.modules == 'trino-redshift' / matrix.profile ==
'cloud-tests'||'fte-tests' to include env.AWS_SECRET_ACCESS_KEY != '' as an
alternative alongside env.AWS_ACCESS_KEY_ID != '' and
env.REDSHIFT_SUBNET_GROUP_NAME != '', preserving the existing
env.CI_SKIP_SECRETS_PRESENCE_CHECKS and env.DEPENDABOT_PR logic so
secrets-provided runs (including Dependabot via secrets) will not be incorrectly
skipped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 20729cfa-1de3-4205-9ec9-8d40e541feb4
📒 Files selected for processing (1)
.github/workflows/ci.yml
| if: >- | ||
| contains(matrix.modules, 'trino-redshift') && | ||
| (contains(matrix.profile, 'cloud-tests') || contains(matrix.profile, 'fte-tests')) && | ||
| (env.CI_SKIP_SECRETS_PRESENCE_CHECKS != '' || env.AWS_ACCESS_KEY_ID != '' || env.REDSHIFT_SUBNET_GROUP_NAME != '') | ||
| (env.CI_SKIP_SECRETS_PRESENCE_CHECKS != '' || ((env.AWS_ACCESS_KEY_ID != '' || env.REDSHIFT_SUBNET_GROUP_NAME != '') && env.DEPENDABOT_PR != 'true')) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the workflow file around lines 771-774
wc -l .github/workflows/ci.ymlRepository: trinodb/trino
Length of output: 1864
🏁 Script executed:
# Read the specific section in question
sed -n '765,780p' .github/workflows/ci.ymlRepository: trinodb/trino
Length of output: 1145
🏁 Script executed:
# Search for other similar Redshift or cloud-test conditions to compare patterns
rg -n "trino-redshift" .github/workflows/ci.yml -A 5 -B 2Repository: trinodb/trino
Length of output: 2454
🏁 Script executed:
# Also search for AWS_SECRET_ACCESS_KEY checks in the workflow
rg -n "AWS_SECRET_ACCESS_KEY" .github/workflows/ci.yml -B 2 -A 2Repository: trinodb/trino
Length of output: 5090
Restore the Redshift secret-backed path.
This is the only updated guard that drops env.AWS_SECRET_ACCESS_KEY != ''. All comparable cloud-test gates (hdfs, s3, delta-lake, iceberg) include this check as an alternative. A Dependabot-triggered run with REDSHIFT_AWS_SECRET_ACCESS_KEY configured will still skip Redshift, even though Dependabot secrets are exposed through the secrets context and the step explicitly defines and uses this secret.
Suggested fix
- (env.CI_SKIP_SECRETS_PRESENCE_CHECKS != '' || ((env.AWS_ACCESS_KEY_ID != '' || env.REDSHIFT_SUBNET_GROUP_NAME != '') && env.DEPENDABOT_PR != 'true'))
+ (env.CI_SKIP_SECRETS_PRESENCE_CHECKS != '' ||
+ ((env.AWS_ACCESS_KEY_ID != '' || env.REDSHIFT_SUBNET_GROUP_NAME != '') && env.DEPENDABOT_PR != 'true') ||
+ env.AWS_SECRET_ACCESS_KEY != '')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 771 - 774, The conditional gating the
trino-redshift cloud tests dropped the AWS secret check and therefore skips
Redshift when only REDSHIFT_AWS_SECRET_ACCESS_KEY is provided (e.g.,
Dependabot). Update the if expression that checks matrix.modules ==
'trino-redshift' / matrix.profile == 'cloud-tests'||'fte-tests' to include
env.AWS_SECRET_ACCESS_KEY != '' as an alternative alongside
env.AWS_ACCESS_KEY_ID != '' and env.REDSHIFT_SUBNET_GROUP_NAME != '', preserving
the existing env.CI_SKIP_SECRETS_PRESENCE_CHECKS and env.DEPENDABOT_PR logic so
secrets-provided runs (including Dependabot via secrets) will not be incorrectly
skipped.
✅ Confirmed as addressed by @Narwhal-fish
There was a problem hiding this comment.
OK I'll add the AWS_SECRET_ACCESS_KEY check back.
There was a problem hiding this comment.
@Narwhal-fish, sounds good! Just to confirm, the updated condition should look like:
(env.CI_SKIP_SECRETS_PRESENCE_CHECKS != '' ||
((env.AWS_ACCESS_KEY_ID != '' || env.REDSHIFT_SUBNET_GROUP_NAME != '') && env.DEPENDABOT_PR != 'true') ||
env.AWS_SECRET_ACCESS_KEY != '')This ensures that Redshift cloud tests can still run when REDSHIFT_AWS_SECRET_ACCESS_KEY is explicitly configured for Dependabot, while also blocking runs where only the variable-backed path (AWS_ACCESS_KEY_ID or REDSHIFT_SUBNET_GROUP_NAME) is visible without the secret.
51d99cc to
e842171
Compare
| SEGMENT_DOWNLOAD_TIMEOUT_MINS: 5 | ||
| CI_SKIP_SECRETS_PRESENCE_CHECKS: ${{ secrets.CI_SKIP_SECRETS_PRESENCE_CHECKS }} | ||
| SECRETS_PRESENT: ${{ secrets.SECRETS_PRESENT }} | ||
| DEPENDABOT_PR: ${{ github.event.pull_request.user.login == 'dependabot[bot]' || github.event.client_payload.pull_request.user.login == 'dependabot[bot]' }} |
There was a problem hiding this comment.
Why have condition on github.event.client_payload.pull_request.user.login? Github docs suggest checking github.event.pull_request.user.login (https://docs.github.com/en/code-security/tutorials/secure-your-dependencies/automating-dependabot-with-github-actions#automatically-approving-a-pull-request)
Add condition on github.repository. This shouldn't have effect in forks (also as suggested by github docs).
There was a problem hiding this comment.
The original reason I used github.event.client_payload.pull_request.user.login was also to support repository_dispatch runs.. I wiil updated it later.
8d18519 to
8b6032a
Compare
Allow Redshift tests to run when only AWS_SECRET_ACCESS_KEY is available (e.g., when configured as a Dependabot secret), matching the pattern used by other cloud-test gates (HDFS, S3, Delta Lake, Iceberg).
8b6032a to
bc7ad66
Compare
|
This pull request has gone a while without any activity. Ask for help on #core-dev on Trino slack. |
Description
Fixes dependabot-originated builds that fail when cloud tests attempt to run without required secrets.
Dependabot can see repository variables but not secrets (unless specifically configured as dependabot secrets). This causes cloud integration tests to execute when
vars.AWS_ACCESS_KEY_IDis present, but then fail whensecrets.AWS_SECRET_ACCESS_KEYis missing, resulting in "Unable to load AWS credentials from environment variables" errors.Fixes #28176
The issue was identified through failed dependabot PR builds where the AWS credentials check passed (because dependabot could see the variable), but the actual test execution failed when attempting to load credentials (because the secret was unavailable).
Release notes
(x) This is not user-visible or is docs only, and no release notes are required.