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

Fix Procfile, update documentation. #64

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

linneakirby
Copy link

Fix Procfile:

  • Before this change, the Procfile only defines the server
  • After this change, the Procfile runs the server.

Update documentation:

  • Before this change, OAUTH_CLIENT_{ID,SECRET} are mentioned in the README,
    but the application actually uses GITHUB_APPLICATION_CLIENT_{ID,SECRET}.

Fix Procfile:
- Before this change, the Procfile only defines the server
- After this change, the Procfile runs the server.

Update documentation:
- Before this change, OAUTH_CLIENT_{ID,SECRET} are mentioned in the README,
  but the application actually uses GITHUB_APPLICATION_CLIENT_{ID,SECRET}.
@timwis
Copy link

timwis commented Dec 23, 2021

So odd, this has stopped working for JKAN installations too, and now I'm wondering how it ever worked, since Procfile only defines the server but doesn't run it.

@linneakirby, I think you may have misread about the GITHUB_APPLICATION_CLIENT_... bits though - if you look through the code, it does use OAUTH_CLIENT_... - the github_ one is only placeholder text. So I think if you remove that change from this PR, perhaps it could be merged.

@bergvall95
Copy link

@timwis I had to look at the code to figure this out myself. I think the language explaining it could be a bit more clear that the keys in the environment variables need to be OAUTH_CLIENT_ID and OAUTH_CLIENT_SECRET. Couldn't figure out why it wasn't working when I was using the GITHUB_APPLICATION.. keys.

@timwis
Copy link

timwis commented Nov 12, 2022

It's been awhile since I was looking at this, but I vaguely recall the issue may have been which branch gets deployed. Development is the default branch, but the deploy button deploys the master branch, I believe.

@bergvall95
Copy link

@timwis I was just talking about the documentation about the names of the environment variables. Pretty long time after to tag you on that, sorry about that!

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.

3 participants