Use different envvars to trigger integration #1086
Conversation
google-cla
bot
added
the
cla: yes
google-oss-robot
added
approved
|
I think we also need to change something in the integration environment for this to work. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mikehelmick, sethvargo The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
mikehelmick
removed
the
approved
|
waiting for word on integration env changes |
internal/integration/helpers.go
Outdated
| if v := os.Getenv("DB_NAME"); v != "" && !testing.Short() { | ||
| dbConfig := &database.Config{} | ||
| var db *database.DB | ||
| if v := os.Getenv("INTEGRATION_DB_NAME"); v != "" { |
There was a problem hiding this comment.
The logic inside this if clause uses DB_NAME env var for establishing database connection from e2e test, I would suggest change this if condition to be
| if v := os.Getenv("INTEGRATION_DB_NAME"); v != "" { | |
| if v := os.Getenv("RUN_E2E_TEST"); v != "" { |
There was a problem hiding this comment.
Based on the discussion at google/exposure-notifications-verification-server#841 (comment), INTEGRATION_DB_NAME would work. Needs several additional changes at:
- Prefix
INTEGRATION_theDB_vars: - Add
env:",prefix:INTEGRATION_"at
internal/integration/helpers.go
Outdated
|
|
||
| var bs storage.Blobstore | ||
| var err error | ||
| if v := os.Getenv("INTEGRATION_GOOGLE_CLOUD_BUCKET"); v != "" { |
There was a problem hiding this comment.
Please also update this line:
I lost a few hours trying to debug why my tests were failing locally. DB_NAME is the envvar automatically set by the dev script, so that triggered the "integration test" path, except my local database already had data, leading to bizarre and very difficult to debug failures. This could have been worse - it's possible someone runs tests when connected to the production database. This changes the environment variables to be prefixed with E2E_ to reduce the chances of such collisions.
sethvargo
force-pushed
the
sethvargo/sad
branch
from
289ae4a
to
ca488a8
Compare
google-oss-robot
added
approved
|
@chaodaiG updated to prefix with |
|
I think this'll work |
|
/unhold |
google-oss-robot
removed
the
do-not-merge/hold
I lost a few hours trying to debug why my tests were failing locally. DB_NAME is the envvar automatically set by the dev script, so that triggered the "integration test" path, except my local database already had data, leading to bizarre and very difficult to debug failures. This could have been worse - it's possible someone runs tests when connected to the production database.
This changes the environment variables to be prefixed with INTEGRATION_ to reduce the chances of such collisions.
/cc @chaodaiG
Release Note