Opened 4 years ago

Closed 4 years ago

#20562 closed enhancement (fixed)

Collector: Edit INSTALL.md and add Nginx config

Reported by: hiro Owned by:
Priority: Low Milestone:
Component: Metrics/CollecTor Version:
Severity: Minor Keywords:
Cc: karsten Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I have tried out CollecTor and added some information to INSTALL.md that others might find helpful.
Specifically I have added a note on how it is possible to use Nginx if this is preferred to Apache.
Patch attached.

Child Tickets

Attachments (1)

expand-install.patch (3.8 KB) - added by hiro 4 years ago.
Add nginx config

Download all attachments as: .zip

Change History (14)

comment:1 Changed 4 years ago by karsten

Thanks for the suggestions! Let's go through them:

  • You write "At the moment, there is no option to limit the amount of disk space used." This implies that there will be such an option in the future. I'm not sure whether that's even possible, nor how to check that. Is that a useful feature, and how do other tools implement something like this? Otherwise, I'd just remove the "At the moment" part.
  • Good catch: "Note that if you haven't edited the contained paths, the script will write to: /srv/collector.torproject.org/collector."
  • Regarding Nginx with FancyIndex module, does that produce the same result as Apache? If so, we should mention it above where we declare Apache as hard dependency and say Nginx (version?) or higher as alternative. If so, which version should we require?

comment:2 Changed 4 years ago by hiro

Thank you for your comment. I have edited the patch.

comment:3 Changed 4 years ago by hiro

I was thinking. We provide .htaccess for Apache. Shall we provide also an Nginx config? I could add it to the patch if we think this might be relevant.

comment:4 Changed 4 years ago by karsten

I'd say, yes, let's also provide an Nginx config if we can keep that small. Want to submit a patch? :)

Please also take a look at the tweaks to your patch in my task-20562 branch and let me know if you're fine with them being squashed into your commit, or what needs more work. Feel free to base your next patch on this branch.

comment:5 Changed 4 years ago by hiro

Yes I am totally happy to squash. I will have to edit the INSTALL.md again w instruction on where to place the nginx config. I think it is better if I had the config to this ticket.

comment:6 Changed 4 years ago by hiro

With this patch I have also added a simple nginx config that allows CollecTor to run on Nginx.

comment:7 Changed 4 years ago by hiro

Summary: Collector: Edit INSTALL.mdCollector: Edit INSTALL.md and add Nginx config

comment:8 Changed 4 years ago by karsten

I think I already had most of your changes in my task-20562 branch. Just pushed another commit to explain where to copy the new nginx-collector file.

However, I didn't find that file in the patch yet. Did you git add it before formatting the patch? Or can you maybe attach it to this ticket and I add it to the branch? Thanks!

comment:9 Changed 4 years ago by hiro

You are right. I forgot to git add. Re-attaching the patch.

Changed 4 years ago by hiro

Attachment: expand-install.patch added

Add nginx config

comment:10 Changed 4 years ago by karsten

Resolution: fixed
Status: newclosed

Looks good! Pushed to my task-20562 branch and pushed a squashed commit to master. Thanks a lot, for trying out the instructions and for preparing and revising this patch! Closing.

comment:11 Changed 4 years ago by iwakeh

Resolution: fixed
Status: closedreopened

Small tweak to make CollecTor comply again to Metrics' Java projects file structure and to avoid that the 'nginx-collector' file is served by the web-server.

Please review this commit.

comment:12 Changed 4 years ago by iwakeh

Status: reopenedneeds_review

comment:13 Changed 4 years ago by karsten

Resolution: fixed
Status: needs_reviewclosed

Merged. Thanks! Closing again.

Note: See TracTickets for help on using tickets.