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

Add heroku #16

Closed
wants to merge 8 commits into from
Closed

Add heroku #16

wants to merge 8 commits into from

Conversation

Taknok
Copy link

@Taknok Taknok commented Sep 25, 2021

📝 Checklist

Make sure that your PR fulfills these requirements:

  • Tests have been added for this feature.
  • All tests pass on your local machine.
  • Code has been linted with the proper rules.

📄 Description

Add button for auto deploy in Heroku and related configuration files.

📌 Does this PR address any issue?

No.

@moonstar-x
Copy link
Owner

Hey there, thanks a lot for your contribution, I'm really happy seeing people using this and wanting to make this better.

However, I'm not really planning on adding a Heroku deploy button because Heroku has an ephemeral filesystem. This is a problem because this bot requires a LevelDB file which acts as a database to save every guild's settings, as well as a cache to see which offers have been notified about already.

This bot is not suitable for Heroku unless the database backend is changed from an embedded to a serviced database, which I am not really planning on implementing yet since it would greatly increase the self-hosted complexity of this project.

I'm sorry, I'm gonna have to close this, but thank you very much for this contribution! 😄

@moonstar-x moonstar-x closed this Sep 25, 2021
@Taknok
Copy link
Author

Taknok commented Sep 25, 2021

True, I did not see that levelDB was used, so yes ephemeral filesystem make the db dumb. If I set a detection for REDIS_URL switching for a RedisDataProvider would that be ok ? Thus, 'default' (without ENV var) levelDB would be used.

@moonstar-x
Copy link
Owner

Yeah, the first thing that came to mind was Redis (at least for the cache part), but there's also the issue that Redis isn't really a persistent data store, more like a key-value memcache, so technically it wouldn't really replace the database.

@Taknok
Copy link
Author

Taknok commented Sep 25, 2021

I do not get "Redis isn't really a persistent data store". Redis does snapshot time to time (~1h). Thus, only last hour will be lose in case of power outage. Not a big deal when self hosting for one/few server in my opinion. For 'medium' hosting, levelDB can be still used. And for a 'large' hosting', a proper database with shards and replicas should be used, but maybe this is not the purpose on short term ^^.

@moonstar-x
Copy link
Owner

Redis isn't persistent as in, if te Redis service is closed, all its data is cleared because Redis stores its data in RAM. I need the data to be persistent, as in, to stay there across restarts.

@Taknok
Copy link
Author

Taknok commented Sep 25, 2021

TO my knowledge, if Redis is properly closed, the data is saved to the disk. If closed by a power outage, only the last snapshot will be used. https://redis.io/topics/persistence

@moonstar-x
Copy link
Owner

You're right, Redis has different backup methods that can somewhat help to persist data, though I still don't think it makes up for a good database alternative because Redis is not a database but a memcache.

In any case, I think that the initial motivation to suggest Redis was for this project to be used in Heroku, but it seems that Redis for Heroku does not support snapshots in its free tier.

Web will idle after 30min without incoming request. Service will run 
permanently.
@Taknok
Copy link
Author

Taknok commented Nov 25, 2021

Hi,

Is it possible to re-open this PR ? I have a version with postgresql which is fully functional (with auto deploy). I tested it over a month and I had all the notifications as the levelDB version running on the discord server moonstar (even null links :/ ).

@moonstar-x
Copy link
Owner

Hi,

Are you implementing a Postgres DataProvider?

I kinda want to keep LevelDB as the primary database because it's really simple to use on self-hosted systems that can have stateful storage.

I don't mind adding support for Postgres as long as its use would implement the DataProvider interface so no changes have to be made in the files that consume client.dataProvider.

In any case, I'm going to re-open this PR but I won't merge it yet because there isn't any change that will make it work on Heroku.

@moonstar-x moonstar-x reopened this Nov 25, 2021
@moonstar-x moonstar-x added the Status: In Progress Currently working on this. label Nov 25, 2021
@moonstar-x
Copy link
Owner

Update: I didn't see the file changes. You must have pushed something while I was writing my reply.

Let me check this during the weekend and I'll let you know.

@Taknok
Copy link
Author

Taknok commented Nov 25, 2021

Postgres is used only if USE_PG is set: https://github.com/Taknok/discord-free-games-notifier/blob/a9ec26c669875d17c2a93aeccb52078ed316ebe0/src/app.js#L43

Update: You can also test it on your own (thk auto deploy ^^) during several weeks, it is not urgent, we have time :)

@Taknok
Copy link
Author

Taknok commented Apr 9, 2022

Bump ?

@moonstar-x
Copy link
Owner

Hi, sorry I haven't been able to check this. College has gotten way too much of my time and I barely have time to maintain this.

The thing is that I need quite some time to check your PR especially because it doesn't contain any tests. :/

@Taknok
Copy link
Author

Taknok commented Aug 25, 2022

https://blog.heroku.com/next-chapter

Well this PR will lose some interest since Heroku stop his free plan...

@moonstar-x
Copy link
Owner

Hi, so I'll be closing this PR for the following reasons:

  1. Inactivity (lol 2 years ago)
  2. Because of Complete Remake [TypeScript, Postgres, Crawler] #26. I've rewritten the whole bot and this PR is no longer relevant because:
    a. The bot now uses Postgres.
    b. You will need external services, notably the crawler, Redis, and Postgres. It will no longer be easy to deploy to Heroku anymore.

@moonstar-x moonstar-x closed this Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: In Progress Currently working on this.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants