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.

Sign up for GitHub

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

Jump to bottom

nixos/buildkite-agent: move to v3 #77950

Merged
merged 7 commits into from
Jan 19, 2020
Merged

nixos/buildkite-agent: move to v3 #77950

merged 7 commits into from
Jan 19, 2020
+126 −63

Conversation

flokli
Copy link
Contributor

@flokli flokli commented Jan 17, 2020

This contains some of the things #77831 and #68378 (which incorporated things from #59358) aim to fix.

Diff to #77831:

  • Above PR makes specifying private and public key optional. However, public keys aren't needed at all to clone private git repos, and with only private keys left, we can simplify the config structure. This also includes the nix store import prevention mechanism suggested by @infinisil in Multiple buildkite agent support #68378 (comment)
  • Instead of supporting two somewhat similar options meta-data and tags, we only support the one supported by version 3.x of the module (as 2.x is dropped here anyways - its latest release has been in January 2018))
  • The bug of setting cfg.package having no effect currently is fixed as well.

Diff to #68378:

  • The cfg.extraSetup command, which would run as root, use deprecated PermissionsStartOnly=yes and requires fixup in preStart was dropped. We still keep cfg.shell, which sets BUILDKITE_SHELL`.
  • The multiple buildkite agent setup isn't included - I find it somewhat complex and would like to discuss a bit more about usage before adding it.
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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.

Sorry, something went wrong.

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jan 17, 2020
mrkkrp
mrkkrp reviewed Jan 17, 2020
View reviewed changes
Copy link
Member

@mrkkrp mrkkrp left a comment

Choose a reason for hiding this comment

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

LGTM, left a comment.

@flokli
Copy link
Contributor Author

flokli commented Jan 17, 2020 via email

@ofborg ofborg bot requested review from ehmry and mostlyobvious January 17, 2020 16:41
@flokli flokli force-pushed the buildkite-3 branch 3 times, most recently from bb00c69 to 6f05f9a Compare January 17, 2020 17:37
@flokli
Copy link
Contributor Author

flokli commented Jan 17, 2020

I adressed the comments, and added the missing release notes. PTAL.

@flokli flokli requested a review from mrkkrp January 17, 2020 17:38
flokli and others added 5 commits January 17, 2020 18:40
@flokli
nixos/buildkite-agent: stop deploying public keys, make ssh private k…
c154dd5 
…ey optional.

SSH public keys aren't needed to clone private repos, and if we only
need to configure a single attribute, there's no need for the "openssh"
attrset anymore.
@flokli
nixos/buildkite-agent: actually use services.buildkite-agent.package
9237767 
We were currently just using pkgs.buildkite-agent, no matter what was
configured in services.buildkite-agent.package
@flokli
nixos/buildkite-agent: tweak TimeoutStopSec and KillMode
99d0f4c 
This improves behaviour when the service is being stopped.
@yorickvP @flokli
buildkite-agent2: remove package

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Learn about vigilant mode
8f19d72 
The latest 2.x release was in Jan 2018.
@yorickvP @flokli
nixos/buildkite-agent: add a shell attribute to specify the shell bei…

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Learn about vigilant mode
e1ad9d1 
…ng used

This gets passed to BUILDKITE_SHELL, which will specify the shell being
used to executes script in.

Defaults to `${pkgs.bash}/bin/bash -e -c`, matching how buildkite
behaves on other distros.
@flokli flokli mentioned this pull request Jan 17, 2020
Closed
10 tasks
@ofborg ofborg bot added 8.has: changelog 8.has: documentation This PR adds or changes documentation labels Jan 17, 2020
zimbatm
zimbatm approved these changes Jan 17, 2020
View reviewed changes
Copy link
Member

@zimbatm zimbatm left a comment

Choose a reason for hiding this comment

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

LGTM

@ofborg ofborg bot requested a review from zimbatm January 17, 2020 22:37
@flokli flokli merged commit eba10dc into NixOS:master Jan 19, 2020
@flokli flokli deleted the buildkite-3 branch January 19, 2020 13:23
@liff liff mentioned this pull request Jun 23, 2021
Open
23 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkaito mkaito

@Lucus16 Lucus16

@jonringer jonringer

@infinisil infinisil

@ehmry ehmry

@mostlyobvious mostlyobvious

@mrkkrp mrkkrp

@zimbatm zimbatm

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: clean-up 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@flokli @zimbatm @mrkkrp @yorickvP