Fix load_samples to correctly collect & load sample connections with "False" secret values #5828
Conversation
NevilleS
added
the
do not merge
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
…d add better DEBUG logs)
NevilleS
changed the title
| # Check if all secret values are present and non-empty | ||
| if all(value and value != "" for value in connection.secrets.values()): # type: ignore | ||
| valid_connections.append(connection) |
There was a problem hiding this comment.
This was the bug: when value == False intentionally, this doesn't work, because all([False, "localhost", "fides", ...]) will be false.
When a secret value isn't set in the ENV, it'll be None (or maybe ""), so this check needed to be fixed. Whoops
| log.info( | ||
| f"Collected {len(valid_connections)} sample connections with configured ENV secrets: {[connection.key for connection in valid_connections]}" | ||
| ) |
There was a problem hiding this comment.
nb. this is the only INFO log in this function, so for regular usage you should just see a log that tells you $N connections were collected and the list of expected connection keys.
DEBUG logs explain why each connection was either included or skipped
| missing_keys = [ | ||
| key | ||
| for key, value in connection.secrets.items() # type: ignore | ||
| if value is None or value == "" |
There was a problem hiding this comment.
This is the right way to check if the secret value was found 👍
| @@ -460,7 +460,7 @@ class TestLoadSamples: | |||
| "FIDES_DEPLOY__CONNECTORS__POSTGRES__DBNAME": "test-var-db", | |||
| "FIDES_DEPLOY__CONNECTORS__POSTGRES__USERNAME": "test-var-user", | |||
| "FIDES_DEPLOY__CONNECTORS__POSTGRES__PASSWORD": "test-var-password", | |||
| "FIDES_DEPLOY__CONNECTORS__POSTGRES__SSH_REQUIRED": "True", | |||
| "FIDES_DEPLOY__CONNECTORS__POSTGRES__SSH_REQUIRED": "false", | |||
There was a problem hiding this comment.
Going forward, this test now reproduces the error we saw when deploying this too
NevilleS
removed
the
do not merge
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (83.33%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #5828 +/- ##
=======================================
Coverage 86.94% 86.94%
=======================================
Files 406 406
Lines 25007 25017 +10
Branches 2695 2695
=======================================
+ Hits 21743 21752 +9
- Misses 2682 2683 +1
Partials 582 582 ☔ View full report in Codecov by Sentry. |
galvana
added
the
run unsafe ci checks
…"False" secret values (#5828)
fides
|
||||||||||||||||||||||||||||
| Project |
fides
|
| Branch Review |
main
|
| Run status |
|
| Run duration | 00m 48s |
| Commit |
|
| Committer | Neville Samuell |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
5
|
| View all changes introduced in this branch ↗︎ | |
Description Of Changes
When
FIDES__DATABASE__LOAD_SAMPLESis set, theload_samplesfunction will run at startup to load sample resources: systems, datasets, etc. It also loads various sample connections (e.g. Postgres, Mongo, Stripe), but it does this conditionally only if the environment is configured with all the expected secrets for each connection.This logic had a subtle footgun; if a secret value was a boolean like
FIDES_DEPLOY__CONNECTORS__POSTGRES__SSH_REQUIRED=False, the code would interpret that as a missing secret and skip the sample connection! Whoops.Code Changes
load_sample_connections_from_project()to only ignoreNoneor""as missing secretsSteps to Confirm
FIDES_DEPLOY__CONNECTORS__POSTGRES__SSH_REQUIRED=Falseand confirm that the Postgres sample connection is configuredFIDES_DEPLOY__CONNECTORS__POSTGRES__SSH_REQUIREDand confirm that the Postgres sample connection is skippedPre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works