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

Multiple buildkite agent support #59358

Closed
wants to merge 6 commits into from
Closed

Multiple buildkite agent support #59358

wants to merge 6 commits into from
+357 −268

Conversation

Lucus16
Copy link
Contributor

@Lucus16 Lucus16 commented Apr 12, 2019 •
edited
Loading

Motivation for this change

For a clearer diff: git diff master:nixos/modules/services/continuous-integration/buildkite-agent.nix HEAD:nixos/modules/services/continuous-integration/buildkite-agents.nix

Buildkite 3 supports multiple agents. We've patched nixpkgs to support this a while ago, but hadn't gotten around to contributing it back yet. There are still a few issues that I'd like opinions and/or contributions for:

  • We removed the buildkite2 package, I'm not sure if you're willing to do that at this time. The buildkite-agent module now forwards to the buildkite-agents which I believe is not compatible with buildkite2.
  • The meta-data string option was removed in favor of the tags attrset option. The openssh options were removed in favor of the optional sshKeyPath option because there seems to be no point in requiring a public key and a private key might not always be necessary either. I believe this may break old configurations.

The initial five patches I've pushed runs in production on our servers, so they're fairly well tested, but if any changes are necessary, we may not be able to test them that easily.

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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Sorry, something went wrong.

@Lucus16 Lucus16 requested a review from infinisil as a code owner April 12, 2019 15:20
@Lucus16
Copy link
Contributor Author

Lucus16 commented Apr 12, 2019

@grahamc @srhb I'd appreciate your opinions.

@Lucus16 Lucus16 mentioned this pull request Apr 12, 2019
Closed
@GrahamcOfBorg GrahamcOfBorg 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/` 8.has: clean-up 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Apr 12, 2019
@yorickvP
Copy link
Contributor

cc: @domenkozar

@domenkozar
Copy link
Member

Not using buildkite anymore sorry :) cc @rvl

@rvl
Copy link
Contributor

rvl commented Jul 12, 2019

@Lucus16 This will be useful. It will need something added to the release notes though, especially for submodule options which have changed (such as sshKeyPath).

It needs a rebase so I tried rebasing the branch on nixos-unstable, but got this error when deploying:

building all machine configurations...
error: The option `services.buildkite-agent' in `/home/rodney/ops/nixpkgs/nixos/modules/services/continuous-integration/buildkite-agent.nix' is a prefix of options in `/home/rodney/ops/nixpkgs/nixos/modules/rename.nix'.
(use '--show-trace' to show detailed location information)

If I removed the change to rename.nix, then I could deploy multiple buildkite agents. I inspected the configs and they look OK.

Backward compatibility for a single-agent config looks good, but I wasn't able to test it.

The documentation string "Attribute set of extra agents." isn't very descriptive. For example, it doesn't say what the attr name does, because each agent entry also has a name.

@rvl rvl mentioned this pull request Jul 13, 2019
Closed
@yorickvP
Copy link
Contributor

PermissionsStartOnly is deprecated: #53852

yorickvP and others added 6 commits August 3, 2019 11:44
@yorickvP @Lucus16
services/buildkite-agent: update service for buildkite 3

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Learn about vigilant mode
fae1962
@lukateras @Lucus16
buildkite-agent: make ssh key optional, remove public key option

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Learn about vigilant mode
37f8265
@yorickvP @Lucus16
buildkite-agent2: drop

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Learn about vigilant mode
d209feb
@Lucus16
buildkite-agent: change option meta-data into tags attrset

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Learn about vigilant mode
bdf692b
@yorickvP @Lucus16
buildkite-agents: add module

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Learn about vigilant mode
906579a
@Lucus16
buildkite-agents: Improve documentation

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Learn about vigilant mode
6de0594
@Lucus16
Copy link
Contributor Author

Lucus16 commented Aug 3, 2019 •
edited
Loading

The change in rename did indeed need to be dropped. I've updated the documentation. Regarding the release note you asked for, is that supposed to go into doc/release-notes.xml, which hasn't been updated since 2012? Should I bump that version and effectively create release 0.15? Just to make sure it's fine if I arbitrarily create a new release like that?

@ivan
Copy link
Member

ivan commented Sep 6, 2019

For the author, reviewers, and committers: this PR was scanned and appears to add a use of the deprecated types.string, which emits a warning as of #66346. Before merging, please change this to another type, possibly:

  • types.str for a single string where merging does not make sense, or cannot work
  • types.lines for multi-line configuration or scripts where merging is possible
  • types.listOf types.str for a mergeable list of strings

@flokli
Copy link
Contributor

flokli commented Sep 7, 2019

@Lucus16 the changes w.r.t. ssh keys make a lot of sense to me. Could you rebase this on latest master, and update the PR regarding types.string and PermissionsStartOnly usage?

@flokli flokli requested a review from zimbatm September 9, 2019 05:05
@zimbatm
Copy link
Member

zimbatm commented Sep 9, 2019

We're using it on the nix-community org. /cc @adisbladis

As long as there is a clear upgrade path, where errors can be resolved by reading the error message and following along, it's fine to break back-compat.

@flokli
Copy link
Contributor

flokli commented Sep 9, 2019 via email

@flokli flokli mentioned this pull request Sep 9, 2019
Closed
10 tasks
@flokli
Copy link
Contributor

flokli commented Sep 9, 2019

Closing in favour of #68378.

@flokli flokli closed this Sep 9, 2019
@flokli flokli mentioned this pull request Jan 17, 2020
Merged
10 tasks
@yorickvP yorickvP mentioned this pull request Jan 23, 2020
Merged
7 tasks
@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
Reviewers

@infinisil infinisil

@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: clean-up 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
10 participants
@Lucus16 @yorickvP @domenkozar @rvl @ivan @flokli @zimbatm @nh2 @GrahamcOfBorg @lukateras