Multiple buildkite agent support #68378
Conversation
ofborg
bot
added
6.topic: nixos
nixos/modules/services/continuous-integration/buildkite-agents.nix
Outdated
Show resolved
Hide resolved
flokli
force-pushed
the
buildkite-agents
branch
from
679b0f0 to
cd0a608
Compare
nixos/modules/services/continuous-integration/buildkite-agents.nix
Outdated
Show resolved
Hide resolved
| type = types.nullOr types.path; | ||
| default = null; | ||
| description = '' | ||
| Private agent SSH key. |
There was a problem hiding this comment.
The best way to prevent this from going into the store is to use toString in an apply directly like
mkOption {
apply = final: if final == null then null else toString final;
}Then you don't need to worry about using the result of this option without toString (and the toString below can be removed).
nixos/modules/rename.nix
Outdated
| @@ -273,6 +273,9 @@ with lib; | |||
| (mkRenamedOptionModule [ "networking" "extraResolvconfConf" ] [ "networking" "resolvconf" "extraConfig" ]) | |||
| (mkRenamedOptionModule [ "networking" "resolvconfOptions" ] [ "networking" "resolvconf" "extraOptions" ]) | |||
|
|
|||
| # Buildkite Agent | |||
| (mkRemovedOptionModule [ "services" "buildkite-agent"] "services.buildkite-agent has been moved to an attribute set at services.buildkite-agents") | |||
There was a problem hiding this comment.
See #61570, I think these renames should be in the module that needs them
There was a problem hiding this comment.
I don't understand the "that needs them".
services.buildkite-agent doesn't exist anymore, you mean I should add those as imports to nixos/modules/services/continuous-integration/buildkite-agents.nix?
There was a problem hiding this comment.
Yeah, like the module that's relevant to the rename/remove
The module has already changed significantly anyways.
It would be weird for it to e.g. allocate a user named foo just because one uses foo as the attribute name.
flokli
force-pushed
the
buildkite-agents
branch
from
cd0a608 to
384b05a
Compare
|
Ok, I think I adressed all the remarks. @infinisil, can you have a second look on it? |
|
@adisbladis, @zimbatm, can you give this a dry run? |
|
All fixes except the multiple buildkite agent support has landed in other PRs by now, and at least I'm fine with only one buildkite agent, so closing. |
😞
😭 |
|
Sorry, this was misunderstanding - I appreciate all the work done here, and PRed most things, it's only that I personally can't pursue the "multiple agents" feature itself. I'd be happy to review a PR by somebody else introducing it 👍 Some notes: As written in #78160, handling multiple users and managing secrets is somewhat hard (also see NixOS/rfcs#59). On top of that, it'd be nice if the service could take use of The current usage of |
|
Concluding comment because PR mentiones do not show up in notifications: This was implemented in #78373. |
Motivation for this change
This is a follow-up of #59358. On top of what's in there, I adressed the requested changes, got rid of
PermissionsStartOnlyanddataDir, and added a NixOS VM test testing the ssh key setup (I can't test connecting to buildkite itself, obviously).Things done
sandboxinnix.confon non-NixOS)nix-shell -p nix-review --run "nix-review wip"./result/bin/)nix path-info -Sbefore and after)Notify maintainers
cc @zimbatm @adisbladis @Lucus16 @rvl @kirelagin