Add static and computed environment variables - #2
Closed
lucperkins wants to merge 4 commits into
Closed
lucperkins wants to merge 4 commits into
lucperkins wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughUpdated README docs to use updated flake schema example and added "Environment variable sets" (static vs computed). Flake outputs changed: added Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flake.nix`:
- Around line 245-255: The computedEnvVars branch calls builtins.attrValues(env)
inside evalChecks.allStrings which can throw on malformed entries before
evalChecks.isAttrs runs; change evalChecks.allStrings so it first checks that
env is an attrset (the same guard logic used for staticEnvVars, e.g. using
evalChecks.isAttrs or the isEnv helper) and only then computes
builtins.attrValues(env) to run the all-strings check, so malformed entries
produce the clean schema error instead of an early exception.
In `@README.md`:
- Around line 174-184: The README example shows
computedEnvVars.${system}.postgres.PGSSLCERT but the actual flake.nix exposes
computedEnvVars.${system}.openssl with OPENSSL_* variables; update the example
to match the flake output by replacing the postgres/PGSSLCERT example with the
openssl entry (computedEnvVars = ... openssl = { OPENSSL_CERT = "..."; ... }) or
otherwise document the openssl key and OPENSSL_* variable names so readers see
the real computedEnvVars.${system}.openssl outputs from flake.nix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+174
to
+184
| Computed sets are system specific and may be based on things like packages in Nixpkgs: | ||
|
|
||
| ```nix | ||
| computedEnvVars = forEachSupportedSystem ( | ||
| { pkgs, system }: | ||
| { | ||
| postgres = { | ||
| PGSSLCERT = "${pkgs.postgresql}/share/postgresql/root.crt"; | ||
| }; | ||
| } | ||
| ); |
There was a problem hiding this comment.
Keep the computed example aligned with the actual flake output.
flake.nix exposes computedEnvVars.${system}.openssl with OPENSSL_* values, but this example documents computedEnvVars.${system}.postgres.PGSSLCERT. As written, readers will look for an output that does not exist.
📝 Proposed doc fix
computedEnvVars = forEachSupportedSystem (
{ pkgs, system }:
{
- postgres = {
- PGSSLCERT = "${pkgs.postgresql}/share/postgresql/root.crt";
+ openssl = {
+ OPENSSL_DIR = "${pkgs.openssl.dev}";
+ OPENSSL_LIB_DIR = "${pkgs.openssl.out}/lib";
+ OPENSSL_INCLUDE_DIR = "${pkgs.openssl.dev}/include";
};
}
);🧰 Tools
🪛 LanguageTool
[grammar] ~174-~174: Use a hyphen to join words.
Context: ...32; }; } ``` Computed sets are system specific and may be based on things like...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 174 - 184, The README example shows
computedEnvVars.${system}.postgres.PGSSLCERT but the actual flake.nix exposes
computedEnvVars.${system}.openssl with OPENSSL_* variables; update the example
to match the flake output by replacing the postgres/PGSSLCERT example with the
openssl entry (computedEnvVars = ... openssl = { OPENSSL_CERT = "..."; ... }) or
otherwise document the openssl key and OPENSSL_* variable names so readers see
the real computedEnvVars.${system}.openssl outputs from flake.nix.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
flake.nix (1)
224-224: DeduplicateisEnvhelper across schema blocks.
isEnvis defined twice with identical logic. Consider hoisting it once in the surroundingschemasscope to reduce drift risk.♻️ Suggested refactor
- schemas = { + schemas = { inherit (inputs.flake-schemas.schemas) devShells formatter overlays schemas ; - } - // { + } + // let + isEnv = v: builtins.isAttrs v && builtins.all (s: builtins.isString s) (builtins.attrValues v); + in { @@ staticEnvVars = { @@ inventory = output: - let - isEnv = v: builtins.isAttrs v && builtins.all (s: builtins.isString s) (builtins.attrValues v); - in inputs.flake-schemas.lib.mkChildren ( @@ computedEnvVars = { @@ inventory = output: - let - isEnv = v: builtins.isAttrs v && builtins.all (s: builtins.isString s) (builtins.attrValues v); - in inputs.flake-schemas.lib.mkChildren (Also applies to: 246-246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flake.nix` at line 224, The helper function isEnv is duplicated in two schema blocks; hoist a single definition up into the surrounding schemas scope and remove the duplicate definitions inside the individual schema blocks so both blocks reference the same isEnv symbol. Locate the existing isEnv definitions (the isEnv = v: builtins.isAttrs v && builtins.all (s: builtins.isString s) (builtins.attrValues v) occurrences), move one copy into the parent schemas scope (above the per-schema blocks), and replace the per-schema copies with references to that hoisted isEnv to avoid drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@flake.nix`:
- Line 224: The helper function isEnv is duplicated in two schema blocks; hoist
a single definition up into the surrounding schemas scope and remove the
duplicate definitions inside the individual schema blocks so both blocks
reference the same isEnv symbol. Locate the existing isEnv definitions (the
isEnv = v: builtins.isAttrs v && builtins.all (s: builtins.isString s)
(builtins.attrValues v) occurrences), move one copy into the parent schemas
scope (above the per-schema blocks), and replace the per-schema copies with
references to that hoisted isEnv to avoid drift.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 176: Change the phrase "system specific" to the hyphenated compound
adjective "system-specific" in the README text (the sentence starting "Computed
sets are system specific and may be based on things like packages in Nixpkgs:")
so the prose uses correct hyphenation for a compound modifier.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54961ee9-f252-4a9e-922a-62a490656f57
📒 Files selected for processing (2)
README.mdflake.nix
🚧 Files skipped from review as they are similar to previous changes (1)
- flake.nix
| } | ||
| ``` | ||
|
|
||
| Computed sets are system specific and may be based on things like packages in Nixpkgs: |
There was a problem hiding this comment.
Hyphenate compound adjective in prose.
Use “system-specific” on Line 176 for correct style.
🧰 Tools
🪛 LanguageTool
[grammar] ~176-~176: Use a hyphen to join words.
Context: ...32; }; } ``` Computed sets are system specific and may be based on things like...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 176, Change the phrase "system specific" to the hyphenated
compound adjective "system-specific" in the README text (the sentence starting
"Computed sets are system specific and may be based on things like packages in
Nixpkgs:") so the prose uses correct hyphenation for a compound modifier.
Member
Author
|
Superseded by #5 |
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
1 participant
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.
Static:
Computed:
Summary by CodeRabbit
Documentation
New Features