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

Fix test-other-modules failure due to lack of Redshift secrets #16484

Merged
merged 2 commits into from
Mar 10, 2023

Conversation

ebyhr
Copy link
Member

@ebyhr ebyhr commented Mar 10, 2023

@cla-bot cla-bot bot added the cla-signed label Mar 10, 2023
@ebyhr ebyhr requested review from findinpath, hashhar and findepi March 10, 2023 04:28
@ebyhr ebyhr added the no-release-notes This pull request does not require release notes entry label Mar 10, 2023
@@ -173,6 +173,30 @@
</dependencies>

<profiles>
<profile>
<id>default</id>
Copy link
Contributor

Choose a reason for hiding this comment

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

The other option would be to add in ci.yml

!:trino-redshift,

after

!:trino-redis,

I'm inclined however to use rather the current option because it gives us extra test-coverage for non cloud-tests that are executed this way on trino-redshift.

@findepi
Copy link
Member

findepi commented Mar 10, 2023

Why would test-other-modules run redshift tests at all?

@ebyhr
Copy link
Member Author

ebyhr commented Mar 10, 2023

@findepi
Copy link
Member

findepi commented Mar 10, 2023

but it should, shouldn't it?

@ebyhr
Copy link
Member Author

ebyhr commented Mar 10, 2023

Added another commit to separate a job.

@@ -538,6 +539,7 @@ jobs:
- { modules: plugin/trino-postgresql }
- { modules: plugin/trino-raptor-legacy }
- { modules: plugin/trino-redis }
- { modules: plugin/trino-redshift }
Copy link
Member

Choose a reason for hiding this comment

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

this will fail.
we should have only { modules: plugin/trino-redshift, profile: cloud-tests }

let's add some comment

Copy link
Member Author

Choose a reason for hiding this comment

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

It works as the screenshot is showing:
Screenshot 2023-03-10 at 23 46 17

@findepi findepi changed the title Exclude Reshift tests by default Fix test-other-modules failure due to lack of Redshift secretts Mar 10, 2023
@findepi findepi changed the title Fix test-other-modules failure due to lack of Redshift secretts Fix test-other-modules failure due to lack of Redshift secrets Mar 10, 2023
@ebyhr ebyhr merged commit 005cf9c into master Mar 10, 2023
@ebyhr ebyhr deleted the ebi/redshift-ci branch March 10, 2023 14:47
@github-actions github-actions bot added this to the 411 milestone Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed no-release-notes This pull request does not require release notes entry
Development

Successfully merging this pull request may close these issues.

4 participants