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

osticket: add module #148959

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

osticket: add module #148959

wants to merge 3 commits into from

Conversation

wmertens
Copy link
Contributor

@wmertens wmertens commented Dec 6, 2021

Motivation for this change

Add osTicket, a nice support ticketing system with email support, as a module.

Things done
To Do
  • move derivation into separate package
  • make OST_CONFIG, like in the mediawiki package
  • do not make cron.php executable
  • override favicon via nginx
  • pick plugins via derivation pkgs.osticket.withPackages (p: with p; [ slack ])

@wmertens
Copy link
Contributor Author

wmertens commented Dec 6, 2021

not sure how to fix the manual build :-(

@aanderse aanderse self-requested a review December 6, 2021 16:59
@wmertens wmertens force-pushed the osticket branch 2 times, most recently from 6869775 to af7b955 Compare December 6, 2021 20:44
echo ${version}
'';

pkg = pkgs.stdenv.mkDerivation rec {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason you're not adding this derivation to the package set and instead putting it directly in the module?

Copy link
Contributor Author

@wmertens wmertens Dec 7, 2021

Choose a reason for hiding this comment

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

the derivation needs the database user etc. Without it, it won't work, and it doesn't make much sense as a stand-alone package.

That's why I thought it would be better together in the module.

I saw the same for mediawiki

Copy link
Member

Choose a reason for hiding this comment

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

In fact mediawiki does have a package. What you linked to was a bit of a wrapper around the package. You should definitely create a separate derivation file in the pkgs/ tree... how else will our bots automatically update your software?

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.

I left some comments... More than I intended to, I guess. I hope they are of value to you. If you need a hand with anything feel free to ping.

Comment on lines +144 to +146
# Patch the location of the config file and the version
sed -i -e "s|INCLUDE_DIR.'ost-config.php'|'${stateDir}/ost-config.php'|" -e "s|define('GIT_VERSION', '\\$git')|define('GIT_VERSION', '${version}')|" bootstrap.php
sed -i "s|../include|${stateDir}|" setup/install.php
Copy link
Member

Choose a reason for hiding this comment

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

A nicer approach here is to leave the application looking at include/ost-config.php, but then create your own version of the file that consists of this:

  <?php
    return require(getenv('OST_CONFIG'));
  ?>

Take a look at the mediawiki package.

Comment on lines +148 to +150
# Pre-populate the database settings for setup.
# The password is not used but required.
sed -i "s|'dbhost'=>'localhost'|'dbhost'=>'localhost','dbuser'=>'${user}','dbpass'=>'${user}','dbname'=>'${user}'|" setup/inc/install.inc.php
Copy link
Member

Choose a reason for hiding this comment

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

Does this imply that the user has to click through a web form to complete installation of this software?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes indeed, there are a bunch of settings to configure and I think automating that ok to keep out of scope for an initial module?

Comment on lines +152 to +153
# Add script header for cron
sed -i '1i\#!/usr/bin/env php' api/cron.php
Copy link
Member

Choose a reason for hiding this comment

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

Upstream doesn't intend for this script to be executable. See below suggestion in definition of systemd.services."${user}-cron".

nixos/modules/services/misc/osticket/default.nix Outdated Show resolved Hide resolved
{
root = mkForce "${pkg}/public";

locations = {
Copy link
Member

Choose a reason for hiding this comment

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

Do you have a link to the documentation on how you got all these locations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alas no, they were extracted by eyeball from the apache configuration files

chmod a+x $out/public/manage.php $out/public/api/cron.php $out/public/api/pipe.php
mkdir -p $out/bin
ln -s ../public/manage.php $out/bin/osticket_manage
ln -s ../public/api/cron.php $out/bin/osticket_cron
Copy link
Member

Choose a reason for hiding this comment

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

No longer required if you accept my suggestion about the cron job.

description = "Issue tracking system";
platforms = platforms.linux;
maintainers = [ maintainers.wmertens ];
license = licenses.gpl2;
Copy link
Member

Choose a reason for hiding this comment

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

gpl2Only or gpl2Plus?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +116 to +117
# TODO extensions function on plugin definition
# that then gets mapped and joined
Copy link
Member

Choose a reason for hiding this comment

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

👍

nixos/modules/services/misc/osticket/default.nix Outdated Show resolved Hide resolved
};

# TODO make this nicer wrt getting the plugins, perhaps ship all plugins by default + allow extra
plugins = mkOption {
Copy link
Member

Choose a reason for hiding this comment

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

It is possible this would be better as an argument to the derivation (when you move that to a separate file) than an option to the module... 🤔 Maybe pkgs.osticket.withPackages (p: with p; [ slack ])? 😅

wmertens and others added 2 commits May 6, 2022 14:32
Co-authored-by: Aaron Andersen <aaron@fosslib.net>
Co-authored-by: Aaron Andersen <aaron@fosslib.net>
@wmertens
Copy link
Contributor Author

wmertens commented May 6, 2022

@aanderse thank you for your thorough review and hints, I distilled them into todos and will address them... this year? 😅

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 12, 2022
@RaitoBezarius
Copy link
Member

The year is soon ending! Can I help you to get this merged? It can be merged the next year if needed :P.

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 22, 2022
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 19, 2024
@wegank wegank marked this pull request as draft March 20, 2024 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants