Skip to content

[SQL Logic Test] Add support for environment variables #5877

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
Merged
merged 8 commits into from
Jan 23, 2023
Merged

[SQL Logic Test] Add support for environment variables #5877

merged 8 commits into from
Jan 23, 2023

Conversation

Tishj
Copy link
Contributor

@Tishj Tishj commented Jan 10, 2023 •
edited
Loading

This PR adds support to the unittest for using environment variables in sql

Previously require-env required 2 arguments:
name and expected_value

Now optionally you can leave out the expected_value, in which case we only verify that the environment variable is defined.

On top of that, we also now register these environment variables and allow tests to use them in SQL

Example

➜  duckdb git:(master) ✗ export env_var_test=1                               
➜  duckdb git:(master) ✗ echo $env_var_test                            
require-env env_var_test

query I
select ${env_var_test}
----
1

Note:
This does not yet work in a result list

query I
select 1;
----
${env_var_test}

Should probably also add this

@Tishj
Copy link
Contributor Author

Tishj commented Jan 10, 2023 •
edited
Loading

Just realized I need to check for name collision with loops/foreach parameters

Also, I thought I was just making this PR to my own fork.. I don't think it's done yet, for one it's missing tests 😅

@Tishj Tishj requested a review from samansmink January 10, 2023 16:19
samansmink
samansmink reviewed Jan 10, 2023
View reviewed changes
Copy link
Contributor

@samansmink samansmink left a comment

Choose a reason for hiding this comment

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

added a small comment, also a test would indeed be nice.

Maybe this is a good opportunity to switch all S3 test cases to use env variables from #5419 instead of the SET variables. Using the new require-env we could instead of checking for S3_TEST_SERVER_AVAILABLE, check for the presence of the AWS credentials in the env. This would also allow easy switching between s3 backends when testing.

@Tishj Tishj requested a review from samansmink January 12, 2023 08:54
@Tishj
Copy link
Contributor Author

Tishj commented Jan 12, 2023 •
edited
Loading

I've adjusted all existing [s3] tests:

  1. to use require-env to require AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY and AWS_DEFAULT_REGION environment variables
  2. removed the initial SET statements for these variables
  3. replaced the hardcoded later SETs (for resetting the variables) to use the ${env_var} functionality

@Mytherin
Copy link
Collaborator

Thanks for the PR! LGTM. @samansmink any more thoughts?

samansmink
samansmink reviewed Jan 16, 2023
View reviewed changes
Copy link
Contributor

@samansmink samansmink left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, looks good! I added one small comment

@Mytherin
Copy link
Collaborator

Looks good from my side, @samansmink any more comments or is this ready to go?

@samansmink
Copy link
Contributor

@Mytherin lgtm!

@Mytherin Mytherin merged commit 2fd5e0d into duckdb:master Jan 23, 2023
@renovate renovate bot mentioned this pull request Feb 28, 2023
Closed
1 task
@renovate renovate bot mentioned this pull request Mar 16, 2023
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@samansmink samansmink samansmink left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Tishj @Mytherin @samansmink