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

Tomcat cleanup && update #18419

Closed
wants to merge 6 commits into from
Closed

Tomcat cleanup && update #18419

wants to merge 6 commits into from

Conversation

danbst
Copy link
Contributor

@danbst danbst commented Sep 8, 2016

Motivation for this change

Update tomcats, add more versions and remove one error in service logs.

Also, webapps folder takes ~40% of package, so I decided to move it to separate output (it isn't a hard requirement for tomcat)

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
    • OS X
    • Linux
  • 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.

I'm going to run all tomcats on my server and then I'll update the WIP status.

@mention-bot
Copy link

@danbst, thanks for your PR! By analyzing the annotation information on this pull request, we identified @sfultong, @eelcovisser and @fpletz to be potential reviewers

@sfultong
Copy link
Contributor

sfultong commented Sep 8, 2016

ACK, looks good 👍

I'm surprised mention-bot mentioned me. I think I made one PR for tomcat ages ago.

@danbst danbst changed the title [WIP] Tomcat cleanup && update Tomcat cleanup && update Sep 9, 2016
@danbst
Copy link
Contributor Author

danbst commented Sep 9, 2016

Removed WIP, ping @svanderburg

@danbst
Copy link
Contributor Author

danbst commented Sep 9, 2016

Bump service default to tomcat85, upstream default. Added changelogs to commit description, TLDR backwards compatible for most of users, but requires java 7 as minimal JDK

@bjornfor
Copy link
Contributor

bjornfor commented Sep 9, 2016

Reviewed, looks good!

@bjornfor
Copy link
Contributor

bjornfor commented Sep 9, 2016

+1 for merge to master and backport to release-16.09.

@danbst
Copy link
Contributor Author

danbst commented Sep 9, 2016

should I squash commits?

@bjornfor
Copy link
Contributor

bjornfor commented Sep 9, 2016

@danbst: Nope, they look nice and clean as is :-)

@bjornfor
Copy link
Contributor

bjornfor commented Sep 9, 2016

Pushed to master (63f9ef9 with parents). Maybe wait a couple of days before backporting to release-16.09?

@bjornfor bjornfor closed this Sep 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants