Conversation
Through a new `environment` option, define your 1Password Environment's ID to either fetch all secrets or specific secrets. Resolves basecamp#1907.
There was a problem hiding this comment.
Pull request overview
Adds 1Password Environment support to secret fetching.
Changes:
- Adds the
--environmentCLI option. - Reads all or selected environment variables.
- Adds environment fetching and authentication tests.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
lib/kamal/cli/secrets.rb |
Exposes and forwards the environment option. |
lib/kamal/secrets/adapters/base.rb |
Extends the adapter fetch interface. |
lib/kamal/secrets/adapters/one_password.rb |
Implements Environment reads and filtering. |
test/secrets/one_password_adapter_test.rb |
Tests Environment fetching and sessions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
lib/kamal/secrets/adapters/one_password.rb:10
- Supplying both source selectors currently makes
--environmentsilently override--from; with no secret names this can return the entire environment even though the caller supplied a narrower vault source. Reject the mutually exclusive options explicitly instead of silently discarding one.
fetch_secrets(secrets, from: from, environment: environment, account: account, session: session)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
lib/kamal/secrets/adapters/one_password.rb:13
- This override no longer satisfies the adapter interface:
Base#requires_account?takes no arguments, so generic callers that invokerequires_account?directly now getArgumentError. Making the environment argument optional preserves the existing contract while still allowing the CLI to pass environment context.
def requires_account?(environment)
lib/kamal/secrets/adapters/one_password.rb:5
--environmentand--fromselect different sources, but when both are supplied the environment branch silently ignores--from. Rejecting this conflicting combination prevents a typo or stale option from fetching secrets from the wrong source.
def fetch(secrets, account: nil, from: nil, environment: nil)
raise RuntimeError, "Missing required option '--account'" if requires_account?(environment) && account.blank?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
lib/kamal/cli/secrets.rb:11
- Validate adapter support before checking its account requirement. As written, an unsupported call such as
--adapter bitwarden --environment asdfwithout--accountreports that the account is missing instead of rejecting--environment; the intended error only appears after the user supplies an otherwise irrelevant account.
if adapter_requires_account?(adapter) && options[:account].blank?
Through a new
environmentoption, define your 1Password Environment's ID to either fetch all secrets or specific secrets from an environment. Resolves #1907.