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

ELK: 5.6.1 -> 5.6.5 & add ELK 6.1.0 #32822

Merged
merged 3 commits into from Jan 14, 2018
Merged

ELK: 5.6.1 -> 5.6.5 & add ELK 6.1.0 #32822

merged 3 commits into from Jan 14, 2018

Conversation

basvandijk
Copy link
Member

@basvandijk basvandijk commented Dec 18, 2017 •
edited

Motivation for this change

This PR upgrades ELK-5 and adds ELK-6.

The test suite is extended to test both ELK-5 and ELK-6.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@srhb
Copy link
Contributor

srhb commented Dec 21, 2017

The index level settings are no longer supported in the elasticsearch config, so we might want to remove them from the extraConf example.

@srhb
Copy link
Contributor

srhb commented Dec 21, 2017

How did you execute the tests? nix-build nixos/release.nix -A tests.elk does not appear to do anything.

@srhb
Copy link
Contributor

srhb commented Dec 21, 2017

Something funny is going on with using "5" and "6" as the names in the attribute set your're mapping over. "elk5" and elk6 works.

@srhb
Copy link
Contributor

srhb commented Dec 21, 2017

This appears to be a Nix 1.12 issue only. Nix 1.11 evaluates it just fine.

@basvandijk
Copy link
Member Author

Hi @srhb, thank you for the review!

I removed the index level settings from the example and I changed the test names "5" and "6" to "ELK-5" and "ELK-6" which should enable them to be evaluated on Nix 1.12.

@basvandijk
Copy link
Member Author

@fpletz any objections against merging this?

@basvandijk
Copy link
Member Author

BTW note that the following succeeds: nix-shell -p nox --run "nox-review pr 32822.

@srhb
Copy link
Contributor

srhb commented Jan 12, 2018

@basvandijk I was under the impression you were waiting for a specific review. If not, I'm happy to merge this. I've used it myself and it looks good. :)

@basvandijk
Copy link
Member Author

@srhb I would appreciate it if you can merge this. Thanks!

I'm also thinking about cherry-picking this on 17.09. I'll open a separate PR for that if you don't beat me to it.

@srhb srhb merged commit ee4e6eb into NixOS:master Jan 14, 2018
@srhb
Copy link
Contributor

srhb commented Jan 14, 2018

Thanks, and please do open a PR for the backport -- I'm unsure on the procedure for adding new major versions back there. :)

@basvandijk
Copy link
Member Author

Thanks Sarah!

@basvandijk basvandijk mentioned this pull request Jan 14, 2018
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@basvandijk @srhb @GrahamcOfBorg