Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple buildkite agent support #68378

Closed
wants to merge 15 commits into from
Closed

Conversation

flokli
Copy link
Contributor

@flokli flokli commented Sep 9, 2019

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 PermissionsStartOnly and dataDir, and added a NixOS VM test testing the ssh key setup (I can't test connecting to buildkite itself, obviously).

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @zimbatm @adisbladis @Lucus16 @rvl @kirelagin

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

(github wants to have something here)

type = types.nullOr types.path;
default = null;
description = ''
Private agent SSH key.
Copy link
Member

Choose a reason for hiding this comment

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

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).

@@ -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")
Copy link
Member

Choose a reason for hiding this comment

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

See #61570, I think these renames should be in the module that needs them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, like the module that's relevant to the rename/remove

pkgs/top-level/all-packages.nix Show resolved Hide resolved
@flokli
Copy link
Contributor Author

flokli commented Sep 10, 2019

Ok, I think I adressed all the remarks. @infinisil, can you have a second look on it?

@lheckemann lheckemann added this to the 20.03 milestone Sep 10, 2019
@flokli
Copy link
Contributor Author

flokli commented Sep 11, 2019

@adisbladis, @zimbatm, can you give this a dry run?

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Code looks good to me, haven't tested it myself though

@flokli
Copy link
Contributor Author

flokli commented Jan 21, 2020

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.

@flokli flokli closed this Jan 21, 2020
@balsoft
Copy link
Member

balsoft commented Jan 21, 2020

and at least I'm fine with only one buildkite agent,

😞

closing.

😭

@flokli
Copy link
Contributor Author

flokli commented Jan 21, 2020

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 DynamicUser=true and StateDirectory=buildkite-agent-${n} (+RuntimeDirectory for configuration file creation).

The current usage of dataDir might prohibit this, but if we break API for the multiple buildkite agent feature anyways, we could remove that option too.

@nh2
Copy link
Contributor

nh2 commented Jun 2, 2020

Concluding comment because PR mentiones do not show up in notifications:

This was implemented in #78373.

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

Successfully merging this pull request may close these issues.

None yet

9 participants