Skip to content

overview: modularize NIX_CONFIG#1097

Merged
erictapen merged 2 commits into
ngi-nix:mainfrom
themadbit:modularize-overview
Jun 6, 2025
Merged

overview: modularize NIX_CONFIG#1097
erictapen merged 2 commits into
ngi-nix:mainfrom
themadbit:modularize-overview

Conversation

@themadbit
Copy link
Copy Markdown
Contributor

No description provided.

@erictapen erictapen self-requested a review June 6, 2025 09:40
Copy link
Copy Markdown
Contributor

@erictapen erictapen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Mark, this is a cool addition to the growing module system of the overview page!

"NIX_CONFIG='${concatSettings}'";
};
settings = mkOption {
type = with types; listOf (submodule ./config-item.nix);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the submodule definition could be inlined here. I think file granularity still needs to be figured out but for now as a rule of thumb stuff that doesn't have a __toString attr shouldn't live in its own file.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually nevermind, just saw what @fricklerhandwerk did in #1092 and it was his idea in the first place, so let's roll with this granularity.

Copy link
Copy Markdown
Contributor

@fricklerhandwerk fricklerhandwerk Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__toString attr shouldn't live in its own file.

It doesn't, but arguably we could have collapsed ./config-item into this module.

@erictapen erictapen enabled auto-merge (squash) June 6, 2025 09:57
@erictapen erictapen merged commit 793b174 into ngi-nix:main Jun 6, 2025
8 checks passed
@github-project-automation github-project-automation Bot moved this to Done in Nix@NGI Jun 6, 2025
Comment on lines +11 to +19
options = {
name = mkOption {
type = types.str;
default = name;
};
value = mkOption {
type = with types; either str (listOf str);
};
};
Copy link
Copy Markdown
Contributor

@fricklerhandwerk fricklerhandwerk Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The NIX_CONFIG thing is a bit tricky. In the instructions what we really want to show is how to set the environment variable to the right value. This is challenging here because NIX_CONFIG is multiline. A convenient rendering will look differently for different shells. So what this change should really be about is

  • adding a shell-code snippet the modular way
  • encoding how to cleverly render setting a multi-line shell variable for bash

the nix configuration we don't need to re-invent -- it's already a NixOS module option, and we should even be able to re-use its rendering logic to produce a string that Nix will accept (not sure though, haven't looked).

then plug it all together such that it displays correctly in the project detail view.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants