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.

Sign up for GitHub

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

Jump to bottom

Add CI infrastructure for trino-redshift #16142

Merged
merged 1 commit into from
Mar 9, 2023
Merged

Add CI infrastructure for trino-redshift #16142

merged 1 commit into from
Mar 9, 2023

Conversation

findinpath
Copy link
Contributor

Description

Use an ephemeral/throw-away Redshift cluster for running integration tests on trino-redshift connector.
Once the tests are run, the testing Redshift cluster is being reclaimed.

The Redshift cluster is publicly accessible in order to be accessible from the general purpose Github runners.

Additional context and related issues

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Feb 16, 2023
@kokosing kokosing mentioned this pull request Feb 17, 2023
Merged
@findinpath findinpath self-assigned this Feb 20, 2023
@findinpath findinpath mentioned this pull request Feb 20, 2023
Closed
@findinpath findinpath requested a review from hashhar February 20, 2023 06:05
@findinpath findinpath force-pushed the findinpath/trino-redshift-ci branch from 3a70aa4 to 98a65ba Compare February 20, 2023 06:05
@findinpath findinpath requested a review from electrum February 20, 2023 06:05
@electrum
Copy link
Member

/test-with-secrets sha=98a65ba757fafc2e537afd2a3368d6a504aeceac

@github-actions
Copy link

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/4267483197

@findepi
Copy link
Member

findepi commented Feb 27, 2023

/test-with-secrets sha=98a65ba757fafc2e537afd2a3368d6a504aeceac

@github-actions
Copy link

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/4281041845

findepi
findepi reviewed Feb 27, 2023
View reviewed changes
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@findinpath findinpath force-pushed the findinpath/trino-redshift-ci branch from 98a65ba to 272ecfa Compare February 27, 2023 14:43
nineinchnick
nineinchnick reviewed Feb 27, 2023
View reviewed changes
.github/bin/redshift/delete-aws-redshift.sh Show resolved Hide resolved
plugin/trino-redshift/README.md Outdated Show resolved Hide resolved
plugin/trino-redshift/README.md Outdated Show resolved Hide resolved
plugin/trino-redshift/pom.xml Outdated Show resolved Hide resolved
@findinpath findinpath force-pushed the findinpath/trino-redshift-ci branch from 272ecfa to fbda6c1 Compare February 27, 2023 21:14
@findinpath findinpath force-pushed the findinpath/trino-redshift-ci branch from fbda6c1 to 8343bf6 Compare February 27, 2023 21:29
@ebyhr
Copy link
Member

ebyhr commented Feb 28, 2023

/test-with-secrets sha=8343bf6bb8118e8beacf8878c331f203e0d146a4

@github-actions
Copy link

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/4290216984

@findinpath
Copy link
Contributor Author

Rebasing on top of master . I'm not sure why the ci / test (plugin/trino-redshift, cloud-tests) (pull_request) is not taken into account.

@findinpath findinpath force-pushed the findinpath/trino-redshift-ci branch from 8343bf6 to 1e48ce9 Compare February 28, 2023 11:33
@ebyhr
Copy link
Member

ebyhr commented Feb 28, 2023 •
edited
Loading

I'm not sure why the ci / test (plugin/trino-redshift, cloud-tests) (pull_request) is not taken into account.

If I remember correctly, the ok to test command doesn't work well when the commit changes ci.yml. Let me create a mirror PR within the repository.

@ebyhr ebyhr mentioned this pull request Feb 28, 2023
Closed
@ebyhr
Copy link
Member

ebyhr commented Feb 28, 2023

Sent #16301 within the repository.

@findinpath
Copy link
Contributor Author

I've removed by mistake the Redshift cluster while the cloud tests were running :( .
I thought it was a problem in the scripts because I was looking at the run from my own PR (and not the copy PR) and this made me confused to think that the cluster has not been deleted as it should after the tests.

@ebyhr could you please issue a new build?

@ebyhr
Copy link
Member

ebyhr commented Feb 28, 2023

Sure, just updated #16301

@findinpath findinpath force-pushed the findinpath/trino-redshift-ci branch from 1e48ce9 to 9f56707 Compare March 7, 2023 09:22
@findinpath findinpath requested a review from findepi March 7, 2023 19:14
@findinpath findinpath force-pushed the findinpath/trino-redshift-ci branch from 9f56707 to 79e411b Compare March 7, 2023 19:17
findepi
findepi approved these changes Mar 8, 2023
View reviewed changes
.github/workflows/ci.yml
REDSHIFT_S3_TPCH_TABLES_ROOT: ${{ vars.REDSHIFT_S3_TPCH_TABLES_ROOT }}
if: >-
contains(matrix.modules, 'trino-redshift') && contains(matrix.profile, 'cloud-tests') &&
(env.AWS_ACCESS_KEY_ID != '' || env.REDSHIFT_SUBNET_GROUP_NAME != '')
Copy link
Member

Choose a reason for hiding this comment

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

there is a SECRETS_PRESENT secret to make these IFs simpler (ie harder to disable something accidentally)

can be a follow-up since this likely pertains other jobs
cc @nineinchnick

plugin/trino-redshift/pom.xml Outdated Show resolved Hide resolved
@findinpath
Add CI infrastructure for trino-redshift
ed24a7f 
Use an ephemeral/throw-away Redshift cluster for running integration tests
on `trino-redshift` connector.
Once the tests are run, the testing Redshift cluster is being reclaimed.

The Redshift cluster is publicly accessible in order to be accessible
from the general purpose Github runners.
@findinpath findinpath force-pushed the findinpath/trino-redshift-ci branch from 79e411b to ed24a7f Compare March 9, 2023 05:37
@findinpath findinpath requested a review from findepi March 9, 2023 05:41
@findepi findepi merged commit fe5a925 into trinodb:master Mar 9, 2023
@github-actions github-actions bot added this to the 411 milestone Mar 9, 2023
@findinpath findinpath deleted the findinpath/trino-redshift-ci branch March 10, 2023 05:07
@colebow colebow mentioned this pull request Mar 14, 2023
Merged
@ssheikin
Copy link
Contributor

@findinpath @nineinchnick could redshift tests run on every merge to master? I bet it could help community to discover some bugs earlier than later?

@nineinchnick
Copy link
Member

They should be, can you confirm that?

@ssheikin
Copy link
Contributor

Ah, may be they were skipped by gib, I've to check.

@nineinchnick
Copy link
Member

Gib is not used on master, we should always run everything. This is a new step in an existing job so we should see if it ran or was skipped.

@nineinchnick
Copy link
Member

They do run: https://github.com/trinodb/trino/actions/runs/4500724776/jobs/7922440389

@findepi
Copy link
Member

findepi commented Mar 24, 2023

could redshift tests run on every merge to master? I bet it could help community to discover some bugs earlier than later?

every merge to master, or every PR towards master?

based on "help community to discover" i assume you meant the latter.
no, PRs cannot run with secrets, so cannot run redshift tests.

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

@nineinchnick nineinchnick nineinchnick approved these changes

@hashhar hashhar Awaiting requested review from hashhar

@electrum electrum Awaiting requested review from electrum

@findepi findepi Awaiting requested review from findepi

Assignees

@findinpath findinpath

Labels
cla-signed
Milestone
411
Development

Successfully merging this pull request may close these issues.
6 participants
@findinpath @electrum @findepi @ebyhr @ssheikin @nineinchnick