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

dynomite: init at 0.6.15 #67243

Closed
wants to merge 2 commits into from
Closed

dynomite: init at 0.6.15 #67243

wants to merge 2 commits into from

Conversation

Scriptkiddi
Copy link
Contributor

Motivation for this change
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 @

@Scriptkiddi Scriptkiddi changed the title init: dynomite at 0.6.14 dynomite: init at 0.6.14 Aug 22, 2019
jonringer
jonringer reviewed Aug 22, 2019
View reviewed changes
pkgs/servers/dynomite/default.nix Outdated Show resolved Hide resolved
pkgs/servers/dynomite/default.nix Outdated Show resolved Hide resolved
@mmahut mmahut requested a review from aanderse August 24, 2019 10:23
@mmahut
Copy link
Member

mmahut commented Aug 24, 2019

Would you like to include a test file maybe? Would be greatly appreciated :)

aanderse
aanderse requested changes Aug 24, 2019
View reviewed changes
Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

Thanks for contributing this package and module. Further to the review left by @mmahut I have left some comments I hope you find useful. Feel more than welcome to ping me if you want clarification on any of the comments I have left.

pkgs/servers/dynomite/default.nix Outdated Show resolved Hide resolved
pkgs/servers/dynomite/default.nix Show resolved Hide resolved
pkgs/servers/dynomite/default.nix Show resolved Hide resolved
nixos/modules/services/cluster/dynomite.nix Outdated Show resolved Hide resolved
nixos/modules/services/cluster/dynomite.nix Outdated Show resolved Hide resolved
pkgs/servers/dynomite/default.nix Outdated Show resolved Hide resolved
nixos/modules/services/cluster/dynomite.nix Outdated Show resolved Hide resolved
nixos/modules/services/cluster/dynomite.nix Outdated Show resolved Hide resolved
nixos/modules/services/cluster/dynomite.nix Show resolved Hide resolved
nixos/modules/services/cluster/dynomite.nix Outdated Show resolved Hide resolved
@Scriptkiddi
Copy link
Contributor Author

Should have fix all the remarks, I wont get to do the test file till the end of the week.

@Scriptkiddi
Copy link
Contributor Author

so apperently dynomite is not supporting YAML 1.2, so I can not use the toYAML for the settings, any idea on how to continue?

@Scriptkiddi Scriptkiddi force-pushed the dynomite_init branch 2 times, most recently from 83509c4 to cff12b7 Compare September 8, 2019 12:45
@Scriptkiddi
Copy link
Contributor Author

Add the test and reverted to the config option since there is no toYAML1.1

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

Sorry for that all done now

@infinisil
Copy link
Member

@GrahamcOfBorg test dynomite

@Scriptkiddi
Copy link
Contributor Author

@infinisil hey could you take another look?

infinisil
infinisil approved these changes Dec 24, 2019
View reviewed changes
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.

Looking about good! Only thing left to do from my side is to clean up the history into an initial commit for the package and another for the module and test

jonringer
jonringer approved these changes Jan 10, 2020
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

git history should be the package and the related module as a commit each, other than that, LGTM

@Scriptkiddi
Copy link
Contributor Author

@jonringer cleaned commit history

@infinisil infinisil force-pushed the dynomite_init branch 2 times, most recently from 0eb52c8 to 9d01374 Compare January 27, 2020 17:14
@infinisil
Copy link
Member

Fixed up the git history myself a bit, also marked aarch64 as broken since ofborg failed for it. Also added the test to all-tests.nix. But it seems that the test is in fact broken, running it yields:

(0.04 seconds)
machine: must succeed: redis-cli -p 8102 ping
machine# [    6.375255] dynomite[669]: [2020-01-27 17:15:22.236] client_unref_internal_try_put:95 <CONN_CLIENT 0x10598d0 -1 from '127.0.0.1:50376'> unref owner <POOL 0x10474d0 'dyn_o_mite'>
machine: exit status 0
(0.02 seconds)
error: Died at (eval 19) line 4, <__ANONIO__> line 671.
(7.14 seconds)
Died at (eval 19) line 4, <__ANONIO__> line 671.
cleaning up
killing machine (pid 597)
(0.00 seconds)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/build/vde1.ctl': Directory not empty
builder for '/nix/store/wz3gwdg7c0kysbb7c9npx0pkgd0vazfz-vm-test-run-dynomite.drv' failed with exit code 255
error: build of '/nix/store/wz3gwdg7c0kysbb7c9npx0pkgd0vazfz-vm-test-run-dynomite.drv' failed

@disassembler disassembler modified the milestones: 20.03, 20.09 Feb 5, 2020
@Scriptkiddi
Copy link
Contributor Author

@infinisil all done

Lassulus
Lassulus reviewed Jun 2, 2020
View reviewed changes
nixos/tests/dynomite.nix
@@ -0,0 +1,35 @@
import ./make-test.nix ({ pkgs, lib, ... }: {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import ./make-test.nix ({ pkgs, lib, ... }: {
import ./make-test-python.nix ({ pkgs, lib, ... }: {

@FRidh FRidh modified the milestones: 20.09, 21.03 Dec 20, 2020
@stale
Copy link

stale bot commented Jun 18, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lassulus Lassulus Lassulus left review comments

@aanderse aanderse aanderse requested changes

@jonringer jonringer jonringer approved these changes

@infinisil infinisil infinisil approved these changes

@mmahut mmahut Awaiting requested review from mmahut

Assignees
No one assigned
Labels
2.status: merge conflict 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: nixos 8.has: module (update) 8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
21.05
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@Scriptkiddi @mmahut @aanderse @infinisil @Lassulus @jonringer @ryantm @lheckemann @disassembler @FRidh