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

startup tweaks and README additions #512

Merged
merged 27 commits into from
Nov 1, 2024

Conversation

slhudson
Copy link
Contributor

These changes address some of the frictions I encountered in getting started that I hope may ease the transition for other potential contributors.

  1. The install.sh script now initializes the users .env file using infra/.env.template. I removed the corresponding snippet from README. I added notes to the template about which parameters are needed for which basic functionality.

  2. I added a create-user.sh script that contributors can execute before running serve.sh so they can upload data and generate visualizations.

  3. I reorganized the README a little to present information in the order a first-time contributor may need it, moving advanced functionality (like movies) toward the bottom and set up details toward the top.

  4. I added missing postregsql and botocore packages I needed to README and requirements-core.txt.

moves examples and oembed into the intro section, which is focused on users who don't want to clone the repo

consolidates movie details at the bottom so first-time users can focus on getting the server running
(1) adds the API credentials and key to the .env.template

(2) creates get-api-key.sh (modeled after serve.sh) to request the API key and store it in .env

(3) modifies the README accordingly
as in testRestApiExampleCode.py
I see now where these values are stored in package.json
 into chore/readme"

This reverts commit 4527891, reversing
changes made to 57ee579.
Copy link
Owner

@artoonie artoonie left a comment

Choose a reason for hiding this comment

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

Thank you! This is the largest community contribution to RCVis, and I greatly appreciate it.

Most of these comments are pretty minor, including some questions about whether things were really required or just came up during the installation attempt.

The main question is around whether create-user.sh provides value over the UI solution -- I could be convinced it does, just want to hear your thoughts on it.

Thank you!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
infra/.env.template Outdated Show resolved Hide resolved
infra/.env.template Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
infra/.env.template Outdated Show resolved Hide resolved
infra/.env.template Outdated Show resolved Hide resolved
infra/.env.template Outdated Show resolved Hide resolved
infra/requirements-core.txt Outdated Show resolved Hide resolved
slhudson and others added 3 commits October 15, 2024 14:02
Co-authored-by: Armin Samii <[email protected]>
Co-authored-by: Armin Samii <[email protected]>
Co-authored-by: Armin Samii <[email protected]>
@slhudson slhudson marked this pull request as draft October 15, 2024 18:27
@slhudson
Copy link
Contributor Author

Thanks so much for your consideration. Most all of your flags and revisions make sense to me.

The one lingering conceptual confusion concerns using the UI to make a new user when running locally. I struggled to do that without the create-user.sh script, but there may well be a better way!

@slhudson slhudson marked this pull request as ready for review October 15, 2024 18:29
@slhudson
Copy link
Contributor Author

slhudson commented Oct 17, 2024 via email

@slhudson
Copy link
Contributor Author

slhudson commented Oct 17, 2024 via email

@artoonie
Copy link
Owner

That's a good suggestion! I'm realizing now that we're overlooking an important step though, which is that the first user should probably be an admin, not a regular user.

For security reasons, I don't want to automate the creation of an admin account -- that should be explicit.

I think the following will achieve your vision:

  1. On ./serve.sh, checks for any admin account
  2. If none exists, it runs python3 manage.py createsuperuser and prompts you to enter a username and password

Thoughts?

By the way -- if you prefer, I'm happy to punt the automation for a separate PR, and just have an instruction that says "run python3 manage.py createsuperuser on first launch" in the README.

@slhudson
Copy link
Contributor Author

slhudson commented Oct 19, 2024 via email

@slhudson
Copy link
Contributor Author

I took a swing at implementing your proposed approach:

  1. On ./serve.sh, checks for any admin account
  2. If none exists, it runs python3 manage.py createsuperuser and prompts you to enter a username and password

Is it alright if we store the username (but not the password) in .env so it can check for the existence of a specific admin account? That's the approach I took here.

If this implementation is alright by you, I think it's pretty straightforward now to go from out-of-the-box to uploading data.

@artoonie
Copy link
Owner

Perfect, thank you! One tiny final suggestion, and we are ready to merge. Really appreciate all the iterations here :)

@slhudson
Copy link
Contributor Author

slhudson commented Oct 21, 2024 via email

@slhudson
Copy link
Contributor Author

Just checking in on next steps with this PR. Is there more needed on my end to tie things up? I'm not sure I understand the continuous-integration/heroku check that's still pending. If there's some more I need to do, please let me know!

scripts/serve.sh Outdated Show resolved Hide resolved
@artoonie
Copy link
Owner

artoonie commented Nov 1, 2024

Sorry for the silence!

  1. Had a pending request which I didn't hit "Submit" on, that task is done now, and
  2. I've been working with Heroku support to try to get Heroku CI working from forks, but unfortunately it's out-of-scope, so I need to figure out a different way of getting the CI to run. More here.

@artoonie artoonie changed the base branch from main to fork/slhudson/chore/readme November 1, 2024 05:08
@artoonie artoonie merged commit 59ade12 into artoonie:fork/slhudson/chore/readme Nov 1, 2024
4 checks passed
artoonie added a commit that referenced this pull request Nov 2, 2024
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.

2 participants