fix(secrets): preserve default_exposed on fal secrets set - #1121
Draft
rehan-remade wants to merge 2 commits into
Draft
fix(secrets): preserve default_exposed on fal secrets set#1121rehan-remade wants to merge 2 commits into
rehan-remade wants to merge 2 commits into
Conversation
`fal secrets set NAME=VALUE` always sent an explicit `default_exposed`, so a plain value rotation overwrote whatever exposure the secret already had. `--not-exposed-by-default` was a bare `store_true`, making `args.not_exposed_by_default` False when the flag was absent, and the value was computed as `not args.not_exposed_by_default`. Since `not <bool>` is always a bool, the CLI could never send `None`. `None` is what the API means by "no opinion". Per the docstring on `SecretsClient.set` in `fal/api/client.py`, it "keeps the account-level default (and preserves the current setting on updates)". Because the CLI never sent it, re-running `set` to rotate a value also reset the secret's exposure, undoing an earlier `--not-exposed-by-default`. Make the exposure flags a tri-state that matches the API: (no flag) sends None, leaving exposure alone --not-exposed-by-default sends False --exposed-by-default sends True (new) Both share `dest="default_exposed"` with `default=None` in a mutually exclusive group, so the existing `--not-exposed-by-default` flag name keeps working. `--exposed-by-default` is new and needed because a bare set no longer implies True, so it would otherwise be impossible to opt a secret back in from the CLI. `BooleanOptionalAction` would be tidier but needs Python 3.9, and this package supports 3.8. The args-to-API translation had no test coverage, which is how this got through in #1103. Add tests asserting the value actually forwarded to `client.secrets.set`, not just the parsed Namespace. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
fal secrets set NAME=VALUEalways sent an explicitdefault_exposed, so using it to rotate a value also reset the secret's exposure setting.--not-exposed-by-defaultwas a barestore_true, soargs.not_exposed_by_defaultisFalsewhenever the flag is absent, and_setcomputed:Since
not <bool>is always a bool, the CLI structurally could never sendNone. ButNoneis what the API means by "no opinion", per the docstring onSecretsClient.setinprojects/fal/src/fal/api/client.py:So the sequence below did not do what it looks like it does. The second command silently undid the first, putting the secret back to being injected into apps that do not list their own
secrets=[...]:Fix
Make the exposure flags a tri-state that matches the API:
fal secrets set A=bNone(leave exposure alone)fal secrets set A=b --not-exposed-by-defaultFalsefal secrets set A=b --exposed-by-defaultTrue(new flag)Both flags share
dest="default_exposed"withdefault=Nonein a mutually exclusive group, so the existing--not-exposed-by-defaultflag name keeps working unchanged.Two notes on the approach:
argparse.BooleanOptionalActionwould be tidier, but it needs Python 3.9 and this package declaresrequires-python = ">=3.8". It would also rename the existing flag.--exposed-by-defaultis new and necessary rather than just convenient: a baresetno longer impliesTrue, so without it there would be no way to opt a secret back in from the CLI.fal secrets listalready renders the third state asaccount defaultwhen the value isNone, so this makes the write side able to express what the read side could already display.Behavior change
Creating a new secret via the CLI now sends
Noneinstead ofTrue, which means the account-level default decides rather than the CLI pinning the secret to exposed.For accounts using the default setting this is equivalent and nothing changes. For an account whose default has been set to not-exposed, newly created secrets are no longer exposed to apps that do not list them, which is the intended meaning of that setting but is a change in observable behavior. Those apps can either declare
secrets=[...]or the secret can be created with--exposed-by-default.Test plan
The args-to-API translation at
cli/secrets.py:13had no coverage: the existing tests asserted only the parsed argparse Namespace, never what reachedclient.secrets.set. That gap is how this got through in #1103. This PR adds tests that assert the forwarded value.pre-commit run --files projects/fal/src/fal/cli/secrets.py projects/fal/tests/unit/cli/test_secrets.py: all hooks pass (ruff-format, ruff, mypy).uv run --extra dev python -m pytest tests/unit/cli/: 6 failed, 224 passed on this branch, versus 6 failed, 218 passed on a pristineorigin/mainworktree. The 6 failures are pre-existing intest_deploy.pyand unrelated to this change, so the delta is exactly the 6 new tests.fal secrets set --helprenders the pair as[--not-exposed-by-default | --exposed-by-default], and passing both errors withargument --exposed-by-default: not allowed with argument --not-exposed-by-default.Not run: integration and e2e tests, per
AGENTS.md, since credentials are not available here. This change was not exercised end to end against a live backend.🤖 Generated with Claude Code