Opened 6 years ago

Closed 3 years ago

#10769 closed enhancement (wontfix)

write weathers emails to a file

Reported by: feverDream Owned by: feverDream
Priority: Medium Milestone:
Component: Metrics/Tor Weather Version:
Severity: Keywords: weather-rewrite
Cc: karsten Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

 so that it doesn't spam regular users during testing.
 

Child Tickets

Change History (11)

comment:1 Changed 6 years ago by feverDream

Status: newassigned
Type: taskenhancement

comment:2 Changed 6 years ago by feverDream

Keywords: weather-rewrite added

comment:3 Changed 6 years ago by karsten

Cc: karsten added

comment:4 Changed 6 years ago by feverDream

emails are sent via send_mass_emails function in updaters.py. So commented it out and the email tuples are written to a file in the applications log directory. Added a link to commit below.

https://github.com/achintangal/weather/commit/271f59755bbd0f1f4d546959a5f203f9210fbc0c

comment:5 Changed 6 years ago by karsten

Commented on GitHub. Thanks!

comment:6 Changed 6 years ago by karsten

Actually, I shouldn't have commented on GitHub, but here. Moving the discussion here again.

So, here's some feedback on https://github.com/achintangal/weather/tree/68035eeea9d26c7f64efd9dbe54845ac4f0e1430:

It looks like Django already provides a convenient way to write email to a file rather than sending it out: https://docs.djangoproject.com/en/1.4/topics/email/#file-backend. Huh! Mind trying that out?

If it works, we shouldn't hack something ourselves. How about we comment out the current email settings that send out actual mail in settings.py and instead put in the file email backend as default? So, whenever we set up weather for local testing or for testing in a local VM using Vagrant, emails would be written to a file. And when we deploy on the server, we'd put in real SMTP settings.

By the way, I somehow like the idea of using different settings files in a settings/ directory for the different environments. I'm just not sure if it's overkill for weather, because the environments aren't that different. But in any case, this discussion deserves a ticket of its own, and the code changes deserve their own branch. Mind opening a ticket and move the discussion there?

comment:7 Changed 6 years ago by feverDream

Nice, I think I should pay more attention to the Django documentation :)

About the settings, I too found the separate module approach a bit clean but I figured its better to ask the other members of the team and see if its actually required.

comment:9 Changed 6 years ago by karsten

There are a few issues with that commit:

  • We cannot comment out settings in weather/config/config.py until we actually moved to Onionoo. What if somebody takes this branch and tries to deploy Weather? It won't work at all.
  • Rather than commenting out code or settings, we should simply remove them. That's for the future, though, when we can actually get rid of them.
  • I'm unclear why you changed the path to the database. That's going to break the existing deployment whenever somebody does git pull. We shouldn't do that. We should rather stick to the existing directory layout with /../var/ even though it's not pretty.
  • Maybe we should also write emails somewhere to /../var/? It seems that Weather currently doesn't write anything to the cloned directory root, but only to /../var/. If that is the case, let's not clutter up /..

So, I guess a commit that only adds the EMAIL_BACKEND and EMAIL_FILE_PATH lines to settings.py but with the right directory to write emails to would be fine to merge.

comment:10 in reply to:  9 Changed 6 years ago by feverDream

Replying to karsten:

There are a few issues with that commit:

  • We cannot comment out settings in weather/config/config.py until we actually moved to Onionoo. What if somebody takes this branch and tries to deploy Weather? It won't work at all.
  • Rather than commenting out code or settings, we should simply remove them. That's for the future, though, when we can actually get rid of them.

Makes sense, but Django expects these files to be a there. I can either go with a dummy authenticator file or a "except: pass" loop around it. I am in favor of the latter.

  • I'm unclear why you changed the path to the database. That's going to break the existing deployment whenever somebody does git pull. We shouldn't do that. We should rather stick to the existing directory layout with /../var/ even though it's not pretty.

I remember changing it as the path was incorrect. To make it work I made these changes;changed the path to PROJECT_PATH:/../../var/WeatherDB and change the permissions of the db-file. For development purposes, I figured the db-file in '/.' was simple.

To keep things intact, making these changes local makes sense to me. What do you think? I can add a few steps in the README describing the settings that need to be changed to run weather locally.

  • Maybe we should also write emails somewhere to /../var/? It seems that Weather currently doesn't write anything to the cloned directory root, but only to /../var/. If that is the case, let's not clutter up /..

Sure thing, I will look into it.

So, I guess a commit that only adds the EMAIL_BACKEND and EMAIL_FILE_PATH lines to settings.py but with the right directory to write emails to would be fine to merge.

Fine, I will trim the commit and move the "settings" stuff to their own ticket( should keeps things separate :))

comment:11 Changed 3 years ago by karsten

Resolution: wontfix
Status: assignedclosed

Tor Weather has been discontinued as of May 24, 2016: https://lists.torproject.org/pipermail/tor-relays/2016-June/009424.html. Batch-closing all remaining tickets as announced in #19382. A list of these tickets and any other Weather tickets modified after June 26, 2016 will be available here: https://trac.torproject.org/projects/tor/query?changetime=Jun+27%2C+2016..&component=^Metrics%2FTor+Weather

Note: See TracTickets for help on using tickets.