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

Heroku button #69

Merged
merged 12 commits into from
Oct 9, 2018
Merged

Heroku button #69

merged 12 commits into from
Oct 9, 2018

Conversation

neveSZ
Copy link
Contributor

@neveSZ neveSZ commented Oct 7, 2018

Files and settings have been added to safely deploy heroku.

@Macleykun
Copy link
Contributor

Macleykun commented Oct 7, 2018

Oh damn those are actually very handy to deploy even easier on heroku! Thanks a lot for this pull dude!

Edit:
I haven’t tested nor seen this confient way before. But would this way allow people to use a different prefix for example?
Maybe make an command to change the prefix on guild level? With fallback for the standard?

Jay or nay?

@neveSZ
Copy link
Contributor Author

neveSZ commented Oct 7, 2018

There are people who have difficulties with the deploy in heroku nowadays, we are not talking only of developers and also of lay users.
I'd say it's not safe for you to leave your bot token in a git heroku. The user could fork the project and edit the settings to make these changes or even give users more customization options with other variables.

synzen
synzen previously requested changes Oct 9, 2018
Copy link
Owner

@synzen synzen left a comment

Choose a reason for hiding this comment

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

This looks really neat. A couple things, if you don't mind:

  • Pull requests should be on dev branch rather than master (I will eventually update the README to clarify this)
  • The env variable DISCORD_TOKEN should have added specificity to be DRSS_DISCORD_TOKEN. This prevents issues for those who uses this as an npm module, so it doesn't use the wrong token by mistake if it's already defined for another app
  • The env variable MONGODB_URI name has the same problem but I understand that it's what heroku sets by default, so for now I guess leave that alone
  • README should clarify that the env variable for the token must be changed on their end in the app's settings on Heroku found next to "Config Vars"
  • Comments added next to the statements using process.env.MONGODB_URI saying that it is intended for use by Heroku

For custom prefixes support, I'll think about it

(nevermind that I didn't actually make any changes to request in this post since it's still on master)

@neveSZ neveSZ changed the base branch from master to dev October 9, 2018 01:57
@synzen
Copy link
Owner

synzen commented Oct 9, 2018

One final thing I forgot to mention - I assume that the deploy to github button must refer to https://github.com/synzen/Discord.RSS/tree/dev instead of https://github.com/synzen/Discord.RSS (for now, until I upgrade versions)

@neveSZ
Copy link
Contributor Author

neveSZ commented Oct 9, 2018

I did not even notice it, I'll fix it, I'd forget it hahahaha. Sorry for any inconvenience I'm a beginner in Github. Thank you. :)

@synzen
Copy link
Owner

synzen commented Oct 9, 2018

Looks good, thanks for the contribution

@synzen synzen merged commit 24b6eef into synzen:dev Oct 9, 2018
@Macleykun
Copy link
Contributor

@neveSZ just curious, but how would this app be updated? Does it freshly clone/check for updates when the app is restarted?

@neveSZ
Copy link
Contributor Author

neveSZ commented Oct 10, 2018

@neveSZ just curious, but how would this app be updated? Does it freshly clone/check for updates when the app is restarted?

Actually no version verification has been added, but you can update by deploying the app manually using the following commands:

$ heroku login
$ heroku git:clone -a {Heroku app name}
$ cd {Heroku app name}
$ git remote add Discord.RSS https://github.com/synzen/Discord.RSS
$ git fetch Discord.RSS dev
$ git reset --hard Discord.RSS/dev
$ git push heroku master --force

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.

None yet

3 participants