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 deployment scripts #116

Merged
merged 6 commits into from Jan 9, 2020
Merged

Conversation

ltalirz
Copy link
Member

@ltalirz ltalirz commented Jan 6, 2020

This PR adds a deployment of the optimade server to heroku - integrated with CI, this allows us to always have a working optimade server of the current development branch (or master, whatever we like) to play around with.
This could be used to e.g. have an "interactive" documentation with this server + swagger (see also Materials-Consortia/materials-consortia.github.io#5 ).

optimade.herokuapp.com is currently linked to my fork of optimade-python-tools but I'll relink it to the main repository once this is reviewed & merged.

Test: https://optimade.herokuapp.com/optimade/structures

@ltalirz ltalirz requested a review from ml-evs January 6, 2020 13:42
Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

This is a great idea and will surely be useful, cheers @ltalirz!

I see it introduces the requirements file again, is this just for heroku to use? Could it be put in e.g. .heroku or somewhere off the top-level to avoid confusion, or are we moving back to using it for all oru deps?

@ltalirz
Copy link
Member Author

ltalirz commented Jan 7, 2020

I see it introduces the requirements file again, is this just for heroku to use?

Yes

Could it be put in e.g. .heroku or somewhere off the top-level to avoid confusion

Not sure, I could try but it needs to be detected by heroku.
One alternative would be to make the "server" extra_requirements part of the default requirements - in that case we can remove the requirements.txt .

For me that would seem fine, since people installing the app will typically want to run the server.
Anyhow, let me know what you guys think @ml-evs @CasperWA

@CasperWA
Copy link
Member

CasperWA commented Jan 8, 2020

One alternative would be to make the "server" extra_requirements part of the default requirements - in that case we can remove the requirements.txt .

For me that would seem fine, since people installing the app will typically want to run the server.

I don't agree. As an example, we (Materials Cloud) do not want to run the server. Although, with the addition of the index meta-database server, we do want to run that.

Maybe with the addition of the index meta-database, which can be properly configured using the server.cfg file, one can justify adding "server" to the core requirements of this package? Although, I still sort of prefer it not to be.

Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

This is a great little tool, thanks @ltalirz!

However, along with @ml-evs I am a bit concerned about the re-addition of the requirements.txt - we essentially already have it under .github/ for GH Actions.
Furthermore, along the lines of @ml-evs's comment, with the re-structuring of the package, it would be nice to put specific heroku files under a dedicated directory if possible.

README.md Outdated
@@ -1,4 +1,5 @@
[![Build Status](https://travis-ci.org/Materials-Consortia/optimade-python-tools.svg?branch=master)](https://travis-ci.org/Materials-Consortia/optimade-python-tools)
[![Heroku](https://heroku-badge.herokuapp.com/?app=optimade&root=optimade/info)](https://optimade.herokuapp.com/optimade)
Copy link
Member

Choose a reason for hiding this comment

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

If this gets merged before #68, could you please make sure this gets added to that PR?

Pipfile breaks heroku build:
       An error occurred while installing 3-5! Will try again.
found no way to specify the 'server' extra on heroku.
this might be a way to install the "server" extra
@ltalirz
Copy link
Member Author

ltalirz commented Jan 8, 2020

ok, I've worked around the requirements.txt issue by fixing the Pipfile instead.
Ready for re-review.

@ml-evs
Copy link
Member

ml-evs commented Jan 8, 2020

Thanks for the changes @ltalirz!

Happy to accept this once we we've merged the readme PR. Quick question in the meantime: how is the heroku auth set up? Is it your personal account or can anyone modify the config? I see they have a github action actions/heroku which might circumvent this (if it's not too much extra effort)?

(I've never used heroku so apologies if these questions are irrelevant)

@ltalirz
Copy link
Member Author

ltalirz commented Jan 8, 2020

Is it your personal account or can anyone modify the config?

Heroku has the concept of "apps" and access can be configured for each app individually.
Happy to add anyone from the optimade github org as maintainers of the app (just let me know the email of your heroku account).

I see they have a github action actions/heroku which might circumvent this (if it's not too much extra effort)?

I don't think this would significantly change things, since in the end you still need to set up the app.

@ml-evs
Copy link
Member

ml-evs commented Jan 8, 2020

I don't think this would significantly change things, since in the end you still need to set up the app.

Okay, cheers! If you can fix the README conflicts then I'll accept.

@ltalirz
Copy link
Member Author

ltalirz commented Jan 8, 2020

Fixed the conflict.
The table now needs two rows for the badges
https://github.com/Materials-Consortia/optimade-python-tools/blob/2283186c631216032ecb6c9d2b7e2334a1b948eb/README.md

I could leave it like this or perhaps remove the "commit activity" one to go back to one line.

@ml-evs
Copy link
Member

ml-evs commented Jan 8, 2020

I could leave it like this or perhaps remove the "commit activity" one to go back to one line.

Up to you, I'll accept either way.

@ml-evs ml-evs self-requested a review January 8, 2020 21:23
@ltalirz ltalirz merged commit 54d1774 into Materials-Consortia:master Jan 9, 2020
@ltalirz
Copy link
Member Author

ltalirz commented Jan 9, 2020

I've now pointed the app to the official optimade-python-tools repo.
Happy to add anyone as a maintainer.

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